Skip to content

fix(platform-wallet)!: complete dashpay#3841

Open
shumkov wants to merge 181 commits into
v3.1-devfrom
feat/dashpay-m1-sync-correctness
Open

fix(platform-wallet)!: complete dashpay#3841
shumkov wants to merge 181 commits into
v3.1-devfrom
feat/dashpay-m1-sync-correctness

Conversation

@shumkov

@shumkov shumkov commented Jun 10, 2026

Copy link
Copy Markdown
Collaborator

Issue being fixed or feature implemented

Milestone 1 of the DashPay completion plan (docs/dashpay/SPEC.md, included in this PR with its research base). DashPay's contact-request flow was broken in four independent, previously-unknown ways:

  1. Every send_contact_request was rejected by consensus — the broadcast carried a document id derived from the creation entropy but fresh entropy in the transition; drive-abci recomputes the id and rejects with InvalidDocumentTransitionIdError.
  2. Wrong encryption wire format — we encrypted the 107-byte DIP-14 ExtendedPubKey::encode() form; DIP-15 and both reference mobile clients (iOS dash-shared-core, Android dashj) use the 69-byte compact fingerprint‖chaincode‖pubkey. Our send failed its own 96-byte ciphertext check; our receive couldn't parse mobile payloads.
  3. Key-purpose incompatibility with mobile clients — verified against all 368 contactRequest documents on testnet: the dominant mobile cohort references an ENCRYPTION key for both key indices (mobile identities carry no DECRYPTION key); our send/validation required DECRYPTION and would fail in both directions.
  4. Sync could not establish contacts — the ingest guard dropped reciprocal requests (offline-accept never established), restore-from-seed permanently bricked Accept (duplicate reciprocal vs the platform unique index), and incoming payments were invisible after restore (receiving account never rebuilt).

What was done?

Three logical commits:

  • docs(dashpay) — the 7-agent-reviewed implementation spec (protocol reference, per-layer inventory, gaps G1–G15, 5-milestone plan, Swift UI design, test plan) + 6 research files including the cross-client interop desk-check and the testnet key-purpose census.
  • fix(sdk)! — entropy threading (ContactRequestResult.entropy reused at broadcast), the DIP-15 69-byte compact-xpub codec in platform-encryption + the SDK callback contract switched to it, and the recipient key-purpose assertion relaxed to DECRYPTION-or-ENCRYPTION.
  • fix(platform-wallet) — new recurring DashPaySyncManager (iterates the wallets map, not the token registry; per-identity log-and-continue); ingest-guard relaxation + sent-side reconcile with idempotent, metadata-preserving merge; Accept adopts an existing on-platform reciprocal instead of re-broadcasting; per-sweep account rebuild (external and receiving accounts) with validate-before-ECDH, guard-drop lock ordering, and a transient/permanent failure policy (payment_channel_broken flag, persisted + FFI accessor); rejected-request tombstone keyed (owner, sender, accountReference) so rotated requests still surface; 69-byte compact parsing on receive with address-equality pinned; key-purpose envelope aligned with on-chain reality; DashPaySdkWriter seam making the write paths testable.

How Has This Been Tested?

TDD throughout — every behavioral fix has a test that was red against the unfixed code and green after (red→green evidence recorded in the SPEC.md M1 DONE notes and the three commit messages):

  • platform-wallet: 196 lib + 8 integration tests green (was 170 before this branch; +34 new)
  • dash-sdk (--features mocks,offline-testing): 139 lib tests green (incl. the entropy-id and 69→96-byte pins)
  • platform-encryption: 7/7 (the crate's test target previously failed to compile — fixed dev-deps)
  • cargo check clean on rs-sdk-ffi, platform-wallet-ffi, platform-wallet-storage; clippy clean on touched crates
  • Live e2e (dp_001..dp_006) is specced to ride the e2e framework in test(platform-wallet): e2e framework + full test suite — triage pins, Found-*/PA-* guards, fail-closed persist, Stage-2 merge #3549 and is explicitly not gated on this PR (SPEC.md Part 7.4)

Note

CI: Rust workspace tests / Tests (macOS) red on 3 pre-existing tests — passing locally.
The macOS check fails only on three receiver-payment tests
(register_contact_account_persists_account_registration,
reconcile_records_received_payments_from_receival_utxos,
reconcile_does_not_clobber_existing_entry_for_same_txid), all with
External signable wallet has no private key.

These pass locally in every configuration tested (9): cargo test, cargo nextest
(isolated and full platform-wallet suite), the CI feature set, --all-features, the
platform-wallet-family feature unification, under cargo llvm-cov coverage, and the
exact CI package set (drive+dpp+drive-abci+… --all-features under coverage) —
all on the same macOS/aarch64 as the runner. All green.

The wallet is provably WalletType::Seed-bearing through every code path (from_seed
Seed; the manager's insert_wallet stores it verbatim; get_wallet returns a &Wallet),
yet only the CI runner reads it as ExternalSignable. Root cause is a use-after-zeroize in
the key-wallet git dependency
: Wallet has a Drop that zeroizes its Zeroize-derived
wallet_type, so the discriminant can corrupt under a particular memory layout (UB is
environment-dependent — it manifests on the CI runner but not locally). This is outside
this PR's code
— pre-existing branch tests plus an external-dependency bug being tracked for
the key-wallet maintainers; the DashPay changes themselves are correct and green.

Breaking Changes

  • rs-sdk: the get_extended_public_key callback contract for create_contact_request/send_contact_request is now "return the 69-byte DIP-15 compact form" (was an encoded ExtendedPubKey); validated before encryption. ContactRequestResult gains a public entropy: Bytes32 field. The rs-sdk-ffi C ABI is unchanged (caller doc contract tightened).
  • platform-wallet storage: schema additions (contacts.payment_channel_broken column, rejected_contact_requests table) in the initial migration; ContactChangeSet gains a rejected field.

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have added "!" to the title and described breaking changes in the corresponding section if my code contains any
  • I have made corresponding changes to the documentation if needed

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Recurring & on-demand DashPay sync with start/stop, status, interval and per-pass summaries (FFI + Swift controls)
    • Full DashPay UI: tab, Contacts, Requests, Contact detail, Add Contact, Send sheet, payment history
    • Local persisted DashPay payment history, device-local contact metadata, contact-info sync/publish, and wallet unlock from keychain mnemonic
  • Bug Fixes

    • DIP‑15 compact xpub interoperability and deterministic contact-request IDs
    • Improved key-purpose validation, payment-channel broken flag, and rejected-request tombstones
  • Documentation

    • Comprehensive DashPay spec, research notes, and interop desk‑check added

shumkov and others added 3 commits June 10, 2026 18:51
…earch

Seven-agent reviewed spec for completing the full DashPay flow (sync, contact
requests, payments, profiles) in the platform wallet + SwiftExampleApp:
protocol reference (DIP-9/11/13/14/15), per-layer implementation inventory,
15 prioritized gaps (G1-G15), 5-milestone work plan, Swift UI design with
normative interaction states, and a two-tier test plan aligned with the
unmerged e2e framework (PR #3549). Backed by 6 source-cited research files,
including the cross-client interop desk-check and an on-chain census of all
368 testnet contactRequest documents.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ompact xpub, key-purpose interop

Three fixes to the rs-sdk/platform-encryption contact-request layer, each
pinned red-to-green:

1. Entropy mismatch (consensus rejection). send_contact_request generated
   fresh entropy for broadcast while the document id was derived from the
   creation entropy; drive-abci recomputes the id from the broadcast entropy
   and rejected EVERY send with InvalidDocumentTransitionIdError.
   ContactRequestResult now carries the creation entropy and send reuses it.
   Test: contact_request_result_entropy_derives_returned_id (red: field
   inexpressible pre-fix; green after).

2. DIP-15 69-byte compact xpub wire format. We encrypted the 107-byte DIP-14
   ExtendedPubKey::encode() form (failing our own 96-byte ciphertext check);
   DIP-15 and both reference mobile clients use fingerprint||chaincode||pubkey
   = 69 bytes. New compact_xpub_bytes/parse_compact_xpub codec in
   platform-encryption; the get_extended_public_key callback contract is now
   the 69-byte compact, validated before encryption. Test:
   test_encrypt_compact_xpub_is_exactly_96_bytes (+ round-trip and
   wrong-length rejection).

3. Key-purpose alignment with on-chain reality. Verified against all 368
   testnet contactRequests: the dominant mobile cohort references an
   ENCRYPTION key for BOTH indices (mobile identities carry no DECRYPTION
   key). The recipient-key assertion now accepts DECRYPTION or ENCRYPTION.
   Test: recipient_key_purpose_accepts_decryption_and_encryption (red on
   DECRYPTION-only predicate; green after).

BREAKING: the SDK-side get_extended_public_key callback must now return the
69-byte DIP-15 compact form (rs-sdk-ffi C ABI unchanged; caller doc
contract tightened). Also enables dashcore/rand in platform-encryption
dev-deps — the crate's tests previously failed to compile at all.

dash-sdk: 139 lib tests green (mocks,offline-testing); platform-encryption
7/7; rs-sdk-ffi check clean.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…blish/reconcile, account rebuild

Milestone 1 of docs/dashpay/SPEC.md. Makes DashPay sync actually converge to
a payable state, recurring, and restore-safe. Each behavior pinned
red-to-green (see SPEC.md Part 5 M1 DONE notes for the full test list).

- Recurring sync (G12): new DashPaySyncManager (modeled on
  PlatformAddressSyncManager) drives dashpay_sync() per wallet on the shared
  cadence/cancel/quiesce machinery — iterating the wallets map, NOT the
  token registry (which skips zero-token identities). Per-identity
  log-and-continue pushed into sync_contact_requests.
  Test: recurring_pass_syncs_every_wallet_including_zero_token_identities.

- Establish via sync (G1a): the ingest guard dropped reciprocal requests
  whose sender we had already sent to — the offline-accept scenario could
  never establish. Guard relaxed; reciprocals now flow into auto-establish.

- Sent-side reconcile (G13): sync now ingests our own on-platform sent
  requests (idempotent, metadata-preserving merge — naive re-establish wiped
  alias/note every sweep), and Accept adopts an existing reciprocal instead
  of re-broadcasting into the unique-index rejection that permanently bricked
  Accept after restore-from-seed.

- Account rebuild sweep (G1b): every established contact missing accounts
  gets validate-key-indices -> decrypt -> register external account, plus the
  DashpayReceivingFunds account (previously only created on fresh send, so
  restore-from-seed left incoming payments invisible). Candidates collected
  under the write guard, registered after guard drop (tokio RwLock is
  non-reentrant).

- Failure policy (G1c): transient failures retry next sweep; permanent
  decrypt/parse failures set the new EstablishedContact.payment_channel_broken
  flag (persisted; FFI accessor added) and stop retrying. Purpose-validation
  mismatches only log-and-skip.

- Reject tombstone (G5 stage 1): rejected requests are tombstoned by
  (owner, sender, accountReference) — never bare sender, so a rotated
  request with a bumped accountReference still gets through. New
  rejected_contact_requests table + ContactChangeSet.rejected.

- Receive-side compact xpub (G14): register_external_contact_account parses
  the 69-byte DIP-15 compact and reconstructs the contact xpub
  (address-equality pinned by reconstructed_xpub_derives_identical_addresses);
  legacy 78/107 fallback kept.

- Key-purpose envelope (G15, verified on-chain): send prefers the
  recipient's DECRYPTION key and falls back to ENCRYPTION (mobile identities
  have no DECRYPTION key); validate_contact_request gains a recipient
  purpose gate (AUTHENTICATION was silently accepted before) and a
  purpose_mismatch classification.

- Testability seam (G11): DashPaySdkWriter object-safe trait over the SDK
  write paths; fetch paths use the SDK's built-in mock.

platform-wallet: 196 lib + 8 integration tests green (was 170);
storage + FFI checks clean; FFI ABI extended by one accessor
(established_contact_is_payment_channel_broken).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds DashPay SPEC/research docs and implements DIP-15 compact-xpub handling, tightened key-purpose validation, rejected-request tombstones and payment-channel-broken tracking, SDK writer seam, recurring DashPay sync manager, incoming-payment recording/reconciliation, FFI extensions (payments/sync/persistence/seed attach), SwiftData models, and SwiftExampleApp UI and tests.

Changes

DashPay Spec & Research

Layer / File(s) Summary
Spec and research docs
docs/dashpay/SPEC.md, docs/dashpay/research/*
Adds master SPEC and research documents covering DIP, keywallet, rs-platform-wallet, SDK/contract, Swift app, and interop desk-check.

Crypto & SDK

Layer / File(s) Summary
Platform encryption: compact xpub & contact-info
packages/rs-platform-encryption/*
Introduce COMPACT_XPUB_LEN, compact xpub assemble/parse, AES helpers for encToUserId/privateData, and tests.
rs-sdk contact-request contract
packages/rs-sdk/src/platform/dashpay/contact_request.rs, packages/rs-sdk-ffi/src/dashpay/contact_request.rs
Require 69-byte DIP‑15 plaintext, add entropy to ContactRequestResult, enforce sender/recipient purpose rules, reuse entropy when sending, and update docs/tests.
Wallet DIP-14/DIP-15 helpers
packages/rs-platform-wallet/src/wallet/identity/crypto/dip14.rs
Add compact_xpub serialization, reconstruct_contact_xpub, account-reference changes, and regression tests.

Validation, State & Storage

Layer / File(s) Summary
Contact validation
packages/rs-platform-wallet/src/wallet/identity/crypto/validation.rs
Classify purpose mismatches with purpose_mismatch flag; sender must be ENCRYPTION, recipient accepts `ENCRYPTION
Changeset & ManagedIdentity
packages/rs-platform-wallet/src/changeset/*, packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/*
Add RejectedContactRequest, rejected changeset map, rejected_contact_requests field and APIs (record_rejected_contact_request, is_request_rejected), idempotent sent handling, and metadata-preserving re-establish.
SQLite schema & migrations
packages/rs-platform-wallet-storage/migrations/*, packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs
Add payment_channel_broken column and rejected_contact_requests table; writer/reader binding updated and tests adjusted.
Apply path
packages/rs-platform-wallet/src/wallet/apply.rs
Replay rejected tombstones into ManagedIdentity state during changeset apply.

FFI & Persistence

Layer / File(s) Summary
FFI contact persistence ABI
packages/rs-platform-wallet-ffi/src/contact_persistence.rs, packages/rs-platform-wallet-ffi/src/persistence.rs
Add payment_channel_broken to ContactRequestFFI, ContactRequestRejectionFFI, extend OnPersistContactsFn signature, and snapshot handling.
FFI payment history
packages/rs-platform-wallet-ffi/src/dashpay_payment.rs, packages/rs-platform-wallet-ffi/src/lib.rs
Expose managed_identity_get_dashpay_payments and deallocator; add module re-exports.
FFI sync bindings
packages/rs-platform-wallet-ffi/src/dashpay_sync.rs
Expose start/stop/status/set-interval/sync_now FFI for DashPay sync manager with pointer validation and tests.
FFI attach seed from mnemonic
packages/rs-platform-wallet-ffi/src/manager.rs
Add platform_wallet_manager_attach_wallet_seed_from_mnemonic FFI and tests; map SeedMismatch.
Contact info setter FFI
packages/rs-platform-wallet-ffi/src/contact_info.rs
Add platform_wallet_set_dashpay_contact_info_with_signer to publish contactInfo with external signer.

SDK writer seam & wallet integration

Layer / File(s) Summary
SDK writer seam
packages/rs-platform-wallet/src/wallet/identity/network/sdk_writer.rs
Add DashPaySdkWriter trait, parameter structs, SignerRef adapter, and SdkWriter production impl.
IdentityWallet wiring
packages/rs-platform-wallet/src/wallet/identity/network/identity_handle.rs, packages/rs-platform-wallet/src/wallet/platform_wallet.rs
Inject sdk_writer into IdentityWallet and init with SdkWriter in PlatformWallet::new; profile flows use sdk_writer.put_document.

Contact flow refactor

Layer / File(s) Summary
Send/sync/accept/reject
packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs, packages/rs-platform-wallet/src/wallet/identity/network/contacts.rs
Send enforces sender key type/purpose, selects recipient key DECRYPTION-first; derive compact xpub bytes; sync fetches sent+received with log-and-continue, dedup, collects account-build candidates, validates before ECDH/register, persists broken-channel flags; accept adopts vs rebroadcast; reject records tombstones; decoding falls back from compact to legacy.

Payments, reconciliation & event bridge

Layer / File(s) Summary
Incoming payment recording
packages/rs-platform-wallet/src/wallet/identity/network/payments.rs, packages/rs-platform-wallet/src/changeset/core_bridge.rs
Implement record_incoming_dashpay_payments to record Received entries from TransactionDetected, add reconcile_incoming_payments local reconciliation; spawn_wallet_event_adapter invokes recorder.

Recurring sync manager

Layer / File(s) Summary
DashPaySyncManager & manager wiring
packages/rs-platform-wallet/src/manager/dashpay_sync.rs, packages/rs-platform-wallet/src/manager/mod.rs, packages/rs-platform-wallet/src/manager/accessors.rs, packages/rs-platform-wallet/src/lib.rs
New coordinator with re-entrancy guard, quiesce semantics, background thread; wire into PlatformWalletManager, add accessors and crate re-exports; dashpay_sync step independence.

Swift SDK and Example App

Layer / File(s) Summary
Swift persistence & handlers
packages/swift-sdk/Sources/SwiftDashSDK/Persistence/*, packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift
Add PersistentDashpayPayment, relation on PersistentIdentity, persistDashpayPayments, persistContacts now accepts rejected snapshots and paymentChannelBroken, callback marshalling updated.
Swift PlatformWallet APIs
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/*.swift
Add getDashPayPayments, DashPay sync control APIs, unlockWalletFromKeychain attach-seed flow, and dashPaySyncIsSyncing state.
SwiftExampleApp UI & tests
packages/swift-sdk/SwiftExampleApp/*, packages/swift-sdk/SwiftExampleApp/SwiftExampleAppUITests/DashPayTabUITests.swift
Add DashPay tab, Contacts/Requests/Add/Detail/Send views, DashPayContactMetaStore, UI wiring to start/stop sync, unit persistence tests, and XCUITests.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Suggested labels

ready for final review

Suggested reviewers

  • lklimek
  • llbartekll
  • ZocoLini
  • thepastaclaw

Poem

"🐇 I nibbled through specs and threaded compact keys,

I traced tombstones where broken channels freeze.
I hop through syncs and tests that hum and play,
Payments march in rows, and UIs show the way.
Hooray — small hops, big fixes, now let the builds sway!"

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/dashpay-m1-sync-correctness

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@thepastaclaw

thepastaclaw commented Jun 10, 2026

Copy link
Copy Markdown
Collaborator

✅ Review complete (commit c8e4065)

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 11

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs (1)

806-875: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

The accept-adopt check is only local, not platform-aware.

already_reciprocated is derived from local sent_contact_requests / established_contacts, but the sync code above explicitly allows "received loaded, sent fetch failed" by logging and continuing. In that state the reciprocal already exists on Platform while already_reciprocated is still false here, so this path retries the same (ownerId, toUserId, accountReference) write and gets the unique-index rejection instead of adopting. This needs a platform check here, or a duplicate-send fallback that switches to the adopt path.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs`
around lines 806 - 875, The local-only already_reciprocated check (variable
already_reciprocated) can be stale; change the flow so before attempting
send_contact_request_with_external_signer you either (A) perform a platform
check for an existing reciprocal contact request/relationship (use whatever
network client/query you have for checking platform contact requests for
(ownerId,toUserId,accountReference)) and set already_reciprocated accordingly,
or (B) keep the existing local check but add a duplicate-send fallback: catch
the unique-index conflict/error returned by
send_contact_request_with_external_signer and, on that specific error, log that
the reciprocal exists on Platform and run the adopt path (call
register_contact_account(&our_identity_id, &sender_id, 0) and treat as success).
Reference already_reciprocated, send_contact_request_with_external_signer, and
register_contact_account when implementing either fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/dashpay/research/01-dip-spec.md`:
- Line 131: Several fenced code blocks use plain ``` without a language tag;
update each triple-backtick fence in the document (e.g., the blocks currently
shown as ``` at the indicated locations) to include an explicit language token
(for non-code or prose use `text`, or a specific language like `json`, `bash`,
`markdown` where applicable) so the markdown linter passes; search for all
occurrences of ``` (including the ones noted around 131, 194, 245, 289, 418,
455) and replace them with ```text or the appropriate language identifier.

In `@docs/dashpay/research/02-rust-dashcore-keywallet.md`:
- Line 232: The markdown contains fenced code blocks without language tags;
update the offending triple-backtick fences to include the appropriate language
identifier (e.g., ```rust, ```bash, or ```text) for the code snippets so
markdownlint passes and syntax highlighting works—locate the plain ``` fences in
the document (the blocks referenced in the review) and replace them with
language-tagged fences.

In `@docs/dashpay/research/05-swift-app.md`:
- Line 47: The fenced code block currently uses a bare triple-backtick fence
(```); add a language tag (e.g., ```swift or ```text) immediately after the
opening backticks to satisfy markdownlint and enable proper syntax highlighting
for that block.

In `@docs/dashpay/research/06-interop-desk-check.md`:
- Line 366: The fenced code block uses plain ``` without a language tag; update
the opening fence to include an appropriate language identifier (for example
`http`, `text`, or `bash`) so markdownlint is satisfied and readability
improves—locate the triple-backtick fenced block in the document and add the
language tag immediately after the opening ``` fence.
- Line 24: The table row contains an extra leading column ("2") so it has four
columns while the table header defines three; remove the extra column in the row
that contains "2" (the row with "ECDH shared-key derivation" and the
libsecp256k1 SHA256 expression) so the row matches the 3-column header layout,
keeping the description "ECDH shared-key derivation" and the verdict "**PASS** —
all three stacks compute libsecp256k1-style `SHA256((y[31]&0x1|0x2) ‖ x)`" as
the remaining columns.

In `@docs/dashpay/SPEC.md`:
- Line 111: Several fenced code blocks in SPEC.md are missing language
identifiers; update each triple-backtick fence (``` ) at the noted examples so
they include an appropriate language tag (e.g., change ``` to ```text, ```rust,
or ```swift as appropriate) to satisfy markdownlint and enable correct syntax
highlighting; search for the bare ``` occurrences (including the ones referenced
near the examples) and replace them with language-tagged fences, ensuring
opening and closing fences remain paired.

In `@packages/rs-platform-wallet/src/manager/dashpay_sync.rs`:
- Around line 221-223: The background loop cleanup currently unconditionally
sets this.background_cancel to None (in the block near start()), which can
overwrite a newer token if stop() and start() race; change the logic so the
background thread only clears background_cancel if the stored cancel token it
captured at spawn time still matches the current token in this.background_cancel
(i.e., capture the Arc/ID of the cancel handle when spawning and
compare-before-clearing); apply the same compare-and-clear pattern in the stop()
/ thread-exit cleanup (references: this.background_cancel, start(), stop()) so a
late-exiting old loop cannot null out a replacement token.

In `@packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs`:
- Around line 103-118: The sender/recipient key selection currently using
sender_identity.public_keys().iter().find(...) (checking Purpose::ENCRYPTION and
KeyType::ECDSA_SECP256K1) can pick a disabled/rotated key; update the logic to
only consider active/enabled keys (e.g., filter by .enabled() or reuse the
existing enabled-key selection utility used for signing) so
sender_encryption_key and recipient_key_index (the call to
select_recipient_key_index should be updated similarly or replaced) always
reference the current active ENCRYPTION/DECRYPTION ECDSA_SECP256K1 key; ensure
you still call .map(...).ok_or_else(...) and preserve error type
PlatformWalletError::InvalidIdentityData when no active key is found.
- Around line 516-556: collect_account_build_candidates currently skips contacts
when info.core_wallet.accounts.dashpay_external_accounts.contains_key(&key) is
true, which prevents retries if register_contact_account previously failed after
inserting an external entry; remove that gating so contacts with an
incoming_request (incoming.encrypted_public_key and key indices) are always
returned as AccountBuildCandidate (unless payment_channel_broken) to allow
build_contact_accounts -> register_contact_account to retry; specifically, in
collect_account_build_candidates remove or change the has_external
check/continue and rely on contact.incoming_request and payment_channel_broken
to decide inclusion (keep AccountBuildCandidate fields: contact_id,
encrypted_public_key, our_decryption_key_index, contact_encryption_key_index).
- Around line 452-509: parse_contact_request_doc currently only extracts
required fields and drops optional fields encryptedAccountLabel and
autoAcceptProof, causing restores to lose these values; update
parse_contact_request_doc (and thus parse_sent_contact_request_doc which calls
it) to also read props.get("encryptedAccountLabel").and_then(|v: &Value|
v.as_str()).map(|s| s.to_owned()) and props.get("autoAcceptProof").and_then(|v:
&Value| v.as_bytes()).cloned() (or appropriate conversions) and pass them into
ContactRequest::new (or the appropriate constructor/factory) so the
ContactRequest created preserves encryptedAccountLabel and autoAcceptProof
during ingest/reconcile. Ensure the match arm pattern includes these Option
values and the fallback logging remains unchanged.

In
`@packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rs`:
- Around line 116-130: The code removes an incoming request from
self.incoming_contact_requests but the returned ContactChangeSet only records
cs.rejected, so on replay the incoming entry isn't removed; update the change
set returned by the function to also include the incoming-removal for (owner_id,
*sender_id, account_reference) (i.e., add the corresponding removal entry to the
ContactChangeSet alongside cs.rejected) so that replay will delete the
incoming_contact_requests entry when applying the rejection tombstone.

---

Outside diff comments:
In `@packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs`:
- Around line 806-875: The local-only already_reciprocated check (variable
already_reciprocated) can be stale; change the flow so before attempting
send_contact_request_with_external_signer you either (A) perform a platform
check for an existing reciprocal contact request/relationship (use whatever
network client/query you have for checking platform contact requests for
(ownerId,toUserId,accountReference)) and set already_reciprocated accordingly,
or (B) keep the existing local check but add a duplicate-send fallback: catch
the unique-index conflict/error returned by
send_contact_request_with_external_signer and, on that specific error, log that
the reciprocal exists on Platform and run the adopt path (call
register_contact_account(&our_identity_id, &sender_id, 0) and treat as success).
Reference already_reciprocated, send_contact_request_with_external_signer, and
register_contact_account when implementing either fix.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 596c3a94-3c49-4cc0-869e-b392a37c181e

📥 Commits

Reviewing files that changed from the base of the PR and between ba94110 and 9f770b8.

📒 Files selected for processing (38)
  • docs/dashpay/SPEC.md
  • docs/dashpay/research/01-dip-spec.md
  • docs/dashpay/research/02-rust-dashcore-keywallet.md
  • docs/dashpay/research/03-rs-platform-wallet.md
  • docs/dashpay/research/04-sdk-and-contract.md
  • docs/dashpay/research/05-swift-app.md
  • docs/dashpay/research/06-interop-desk-check.md
  • packages/rs-platform-encryption/Cargo.toml
  • packages/rs-platform-encryption/src/lib.rs
  • packages/rs-platform-wallet-ffi/src/established_contact.rs
  • packages/rs-platform-wallet-storage/migrations/V001__initial.rs
  • packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs
  • packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs
  • packages/rs-platform-wallet-storage/tests/sqlite_load_reconstruction.rs
  • packages/rs-platform-wallet/src/changeset/changeset.rs
  • packages/rs-platform-wallet/src/changeset/mod.rs
  • packages/rs-platform-wallet/src/lib.rs
  • packages/rs-platform-wallet/src/manager/accessors.rs
  • packages/rs-platform-wallet/src/manager/dashpay_sync.rs
  • packages/rs-platform-wallet/src/manager/mod.rs
  • packages/rs-platform-wallet/src/wallet/apply.rs
  • packages/rs-platform-wallet/src/wallet/identity/crypto/dip14.rs
  • packages/rs-platform-wallet/src/wallet/identity/crypto/validation.rs
  • packages/rs-platform-wallet/src/wallet/identity/network/account_labels.rs
  • packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs
  • packages/rs-platform-wallet/src/wallet/identity/network/contacts.rs
  • packages/rs-platform-wallet/src/wallet/identity/network/dashpay_sync.rs
  • packages/rs-platform-wallet/src/wallet/identity/network/identity_handle.rs
  • packages/rs-platform-wallet/src/wallet/identity/network/mod.rs
  • packages/rs-platform-wallet/src/wallet/identity/network/profile.rs
  • packages/rs-platform-wallet/src/wallet/identity/network/sdk_writer.rs
  • packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rs
  • packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/identity_ops.rs
  • packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/mod.rs
  • packages/rs-platform-wallet/src/wallet/identity/types/dashpay/established_contact.rs
  • packages/rs-platform-wallet/src/wallet/platform_wallet.rs
  • packages/rs-sdk-ffi/src/dashpay/contact_request.rs
  • packages/rs-sdk/src/platform/dashpay/contact_request.rs

Comment thread docs/dashpay/research/01-dip-spec.md Outdated
Comment thread docs/dashpay/research/02-rust-dashcore-keywallet.md Outdated
Comment thread docs/dashpay/research/05-swift-app.md Outdated
Comment thread docs/dashpay/research/06-interop-desk-check.md Outdated
Comment thread docs/dashpay/research/06-interop-desk-check.md Outdated
Comment thread packages/rs-platform-wallet/src/manager/dashpay_sync.rs Outdated
@codecov

codecov Bot commented Jun 10, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 92.27468% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.20%. Comparing base (96cba16) to head (5d26736).
⚠️ Report is 1 commits behind head on v3.1-dev.

Files with missing lines Patch % Lines
packages/rs-platform-encryption/src/lib.rs 78.12% 14 Missing ⚠️
...s-platform-wallet-storage/src/sqlite/migrations.rs 92.85% 2 Missing ⚠️
...tform-wallet-storage/src/sqlite/schema/contacts.rs 98.92% 1 Missing ⚠️
...-wallet-storage/src/sqlite/schema/identity_keys.rs 97.22% 1 Missing ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##           v3.1-dev    #3841       +/-   ##
=============================================
+ Coverage     52.54%   87.20%   +34.65%     
=============================================
  Files            11     2632     +2621     
  Lines          1707   327792   +326085     
=============================================
+ Hits            897   285848   +284951     
- Misses          810    41944    +41134     
Components Coverage Δ
dpp 87.72% <ø> (∅)
drive 86.14% <ø> (∅)
drive-abci 89.45% <ø> (∅)
sdk ∅ <ø> (∅)
dapi-client ∅ <ø> (∅)
platform-version ∅ <ø> (∅)
platform-value 92.20% <ø> (∅)
platform-wallet ∅ <ø> (∅)
drive-proof-verifier 49.55% <ø> (∅)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

M1 of the DashPay completion plan: the SDK entropy / DIP-15 compact xpub / key-purpose interop fixes are correct, but six in-scope correctness issues block merge. The most concerning is editing V001 in-place (violates the documented append-only migration policy and bricks DB rehydration for the v4.0.0-beta.4 cohort). Additional blockers: the reject path emits an incomplete ChangeSet (no removed_incoming); the new rejected_contact_requests table is written but never read; transient identity fetches in register_external_contact_account are misclassified as permanent and brick the channel; validation.purpose_mismatch is set even when a hard error is also present, masking permanent failures as retryable; and the sync sweep skips superseding requests from established contacts, making the documented payment_channel_broken recovery path unreachable.

🔴 6 blocking | 🟡 2 suggestion(s)

2 additional finding(s) omitted (not in diff).

🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-platform-wallet-storage/migrations/V001__initial.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/migrations/V001__initial.rs:186-213: V001 migration edited in-place violates append-only policy and breaks upgrade from v4.0.0-beta.4
  This PR adds `contacts.payment_channel_broken` and a new `rejected_contact_requests` table by editing V001 directly. V001 (without these additions) was already shipped in `v4.0.0-beta.4` (commit da9d3fe84e / schema confirmed via `git show`), and `packages/rs-platform-wallet-storage/README.md:106` explicitly states migrations are append-only and applied by refinery on every `open`. refinery checksums each migration in `refinery_schema_history`; against an existing v4.0.0-beta.4 DB it will either abort with a divergent-checksum error or silently skip V001 (already applied) — in which case neither the new table nor the new column is ever created, and the first runtime write in `contacts.rs:240` (`INSERT INTO rejected_contact_requests …`) or `contacts.rs:194-212` (`payment_channel_broken` column) fails at the SQLite layer. `tc029_migration_fingerprint_stable` does not catch this because it only checks self-stability, not a pinned hash. Add `V002__dashpay_reject_and_broken_channel.rs` doing `ALTER TABLE contacts ADD COLUMN payment_channel_broken INTEGER` and `CREATE TABLE rejected_contact_requests (…)`; the loader at `contacts.rs::load_state` already tolerates NULL `payment_channel_broken`, so a default-less ALTER is compatible.

In `packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rs:109-131: `record_rejected_contact_request` removes incoming in memory but does not emit `removed_incoming`
  The function calls `self.incoming_contact_requests.remove(sender_id)` and returns a `ContactChangeSet` populated only with `cs.rejected`. The unified-`contacts`-table writer at `rs-platform-wallet-storage/src/sqlite/schema/contacts.rs:182-193` only `DELETE`s when `cs.removed_incoming` is non-empty, so the previously persisted state='received' row (with the `incoming_request` blob) stays in SQLite. Once `persister.load()` (TODO at `sqlite/persister.rs:909`) is wired up, the unified contacts reader rebuckets that row as an incoming request, `apply_changeset` re-inserts it into `incoming_contact_requests`, and the FFI surfaces the explicitly-rejected request back to the UI. The persisted delta is also internally inconsistent with the in-memory mutation — a delta-persistence invariant violation. The in-memory `rejected_tombstone_round_trips_and_respects_account_reference` test does not catch this because it round-trips via `apply_changeset`, not the SQLite reader. Fix by also inserting a matching `ReceivedContactRequestKey { owner_id, sender_id: *sender_id }` into `cs.removed_incoming`.

In `packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs:206-222: `rejected_contact_requests` is written but never read — tombstones lost across restart
  The PR adds a writer (`contacts.rs:240`), a migration row (`V001__initial.rs:203`), and an `apply_changeset` branch that restores `ManagedIdentity.rejected_contact_requests` from `cs.rejected`. But `managed_identity_from_entry` hard-codes `rejected_contact_requests: Default::default()` (line 214), and grep confirms no `load_state` reader for the new table. Once `persister.load()` (TODO at `sqlite/persister.rs:909`) is wired up, the in-memory tombstone map is always empty after restart even though SQLite holds the rows. `is_request_rejected` then returns `false`, the sweep's tombstone-skip in `network/contact_requests.rs:396-404` does not fire, and the recurring DashPay loop (G12) resurrects every rejected request on the first sweep — exactly the M1 failure mode the SPEC.md cites as the reason G5 must land with G12. Add a per-wallet `load_state` for `rejected_contact_requests` and route its output into the `ContactChangeSet.rejected` synthesized during rehydration.

In `packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs:711-728: Transient identity fetch failures inside account-build are marked as permanent
  `build_contact_accounts` treats any error from `register_external_contact_account` as permanent and calls `mark_contact_channel_broken`. But `register_external_contact_account` (`network/contacts.rs:400-407`) performs another `Identity::fetch` for the same contact and wraps the DAPI/network error as `PlatformWalletError::InvalidIdentityData`. A transient DAPI hiccup after validation therefore permanently disables the payment channel; subsequent sweeps skip the contact via the `payment_channel_broken` filter at line 530, and recovery only fires if a superseding contactRequest happens to arrive — contradicting the policy in the docstring at lines 573-578 ("Transient (identity fetch / network): logged, left for the next sweep to retry. The broken flag stays clear."). The fix is to perform the contact-identity fetch and treat its failure as transient *before* calling `register_external_contact_account` (mirroring the existing fetch at lines 631-655) and to scope the permanent-broken classification to genuinely non-recoverable failures (decrypt/decode, missing-key, key-type mismatch).
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs:383-392: Superseding requests from established contacts are skipped — `payment_channel_broken` recovery is unreachable
  The received-side ingest drops every doc whose sender is already in `established_contacts` before consulting `accountReference`. `EstablishedContact::reestablish_preserving_metadata` exists precisely to clear `payment_channel_broken = false` when a fresh request flows in (see `types/dashpay/established_contact.rs:84-104`), and `collect_account_build_candidates` documents the recovery contract at lines 528-529 ("never retry a permanently-broken channel — wait for a superseding request (which clears the flag on re-establish)"). But there is no path that reaches `reestablish_preserving_metadata` for an already-established sender from the sync sweep — `add_incoming_contact_request` is only called for new senders here, and the send-side guard at `state/managed_identity/contact_requests.rs:46-48` similarly returns early for established contacts. Net effect: once `payment_channel_broken` is set, it stays set forever. Either (a) detect a superseding incoming request (new `accountReference` for the same sender) and route it through the reestablish path, or (b) change the broken-channel policy so the next sweep can retry under controlled conditions.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs:5733-5749: `select_recipient_key_index` returns disabled keys
  The send-side recipient-key selector iterates `recipient_identity.public_keys()` and returns the first key whose purpose is DECRYPTION (then ENCRYPTION) and whose type is ECDSA_SECP256K1, with no `disabled_at` check. `validate_contact_request` in `crypto/validation.rs` does gate on disabled keys, so if a preferred DECRYPTION key has been rotated/disabled this selector returns it anyway and the broadcast fails downstream with an opaque error instead of falling through to a usable ENCRYPTION key on the same identity. Add `&& k.disabled_at().is_none()` to both branches so selection is consistent with validation.

In `packages/rs-platform-wallet/src/wallet/identity/crypto/validation.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/crypto/validation.rs:50-62: `purpose_mismatch` is set even when a non-purpose hard error is also present
  The docstring at lines 19-29 contracts `purpose_mismatch` as `true` *only* when the sole reason for invalidity is a key-purpose mismatch — it is what tells `build_contact_accounts` at `network/contact_requests.rs:689` to treat the failure as a non-permanent skip instead of marking the channel permanently broken. The implementation does not preserve that invariant: `add_purpose_error` unconditionally sets `purpose_mismatch = true`, and `add_error` never clears it. A request whose key has both a wrong key type (hard, permanent error) and a wrong purpose ends up with `is_valid = false, purpose_mismatch = true`, and the caller skips + retries forever instead of marking broken. The mobile testnet census makes this rare in practice today, but the classifier is the load-bearing primitive the recovery policy is built on — fix `add_error` to clear the flag, and fix `add_purpose_error` to only set it if no prior hard error was recorded.

In `packages/rs-platform-wallet/src/manager/dashpay_sync.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/manager/dashpay_sync.rs:192-227: `stop()` followed quickly by `start()` can let the old thread null out the new cancel token
  `stop()` takes the current `background_cancel`, sets it to `None`, then cancels the old token. The spawned thread exits its loop on cancellation and at lines 221-223 re-acquires the guard and writes `*guard = None`. If `start()` runs between `stop()` and the old thread's cleanup block, the old thread's final clear will overwrite the new token just installed by `start()` — leaving `background_cancel` empty while a fresh sync thread is still running, so a subsequent `stop()`/`quiesce()` will be a no-op against that running thread. The normal shutdown path (`quiesce` waits for in-flight passes) does not hit this, but bare `stop()`/`start()` races can. Fix by capturing the token at spawn time and only clearing the guard if it still holds that same token (`if matches!(*guard, Some(t) if Arc::ptr_eq(...)) { *guard = None; }`).

Comment thread packages/rs-platform-wallet-storage/migrations/V001__initial.rs
Comment thread packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs Outdated
Comment thread packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs Outdated
Comment thread packages/rs-platform-wallet/src/manager/dashpay_sync.rs
shumkov added 6 commits June 12, 2026 21:31
…hout order-by

Two devnet-UAT fixes on the rs-sdk side:

- contact_request_queries: add explicit `ORDER BY $createdAt` to both
  fetch_received/fetch_sent queries. Drive answers a bare
  secondary-index equality (toUserId / $ownerId) with a verified
  proof of ABSENCE even when matching documents exist — isolated
  live against devnet with a host-side probe (equality-only: 0 docs;
  with order-by: found). The order-by binds the query to the
  (field, $createdAt) index so results return. Worth a platform
  issue: drive should reject the under-specified query instead of
  proving absence.

- rs-sdk-ffi: 8MB tokio worker stacks. GroveDB document-query proof
  verification (verify_layer_proof_v1) recurses deep enough to
  overflow the platform-default stack (SIGBUS on the stack guard,
  observed on-device).

No test: requires a live drive node answering proofs; pinned by the
on-device UAT flow (docs/dashpay/SPEC.md Part 7 e2e plan covers it
once PR #3549 lands).
…lock

Devnet UAT (2026-06-12) showed the receiver's payment history was
always empty ("Payments (0)") and friendship-account UTXOs were
silently dropped on every relaunch. Three root causes, all fixed:

1. Incoming payments were never recorded: the old
   try_record_incoming_payment had ZERO callers. Replaced with
   record_incoming_dashpay_payments wired into the wallet-event
   adapter (core_bridge) — every TransactionDetected output paying a
   DashpayReceivingFunds address now records a Received PaymentEntry
   on the owning managed identity, idempotent per txid.

2. No recovery for missed/restored payments: new
   reconcile_incoming_payments() derives missing Received entries
   from the receival accounts' UTXO sets; runs as a local-only third
   step of dashpay_sync() each sweep. Never clobbers an existing
   txid entry (e.g. the sender's own Sent record when both
   identities share a wallet).

3. DashPay account registrations were in-memory only:
   register_contact_account / register_external_contact_account now
   persist an AccountRegistrationEntry + initial pool snapshot (same
   round shape as wallet creation), emitted BEFORE the in-memory
   inserts. Without this the accounts vanished on relaunch and the
   UTXO restore dropped their rows (load: dropped_no_account=2
   observed live). register_contact_account also gains the missing
   early-exit and now mirrors the restored shape into the immutable
   wallet.accounts collection.

Tests (red->green demonstrated against the unfixed code):
- register_contact_account_persists_account_registration: FAILED
  before (no store round), passes after.
- reconcile_records_received_payments_from_receival_utxos: FAILED
  before (stub recorded 0), passes after; also pins idempotency.
- reconcile_does_not_clobber_existing_entry_for_same_txid.
204/204 platform-wallet lib tests green.

Also: attach_wallet_seed manager API + FFI
(platform_wallet_manager_attach_wallet_seed_from_mnemonic) — wallets
rehydrate external-signable after relaunch with the mnemonic still
in the host keychain; this upgrades them in place (idempotent,
SeedMismatch-guarded, BIP44-0 xpub-equality fallback for
pre-network-scoped wallet ids). dashpay-sync loop thread gets an
8MB stack (GroveDB proof recursion SIGBUS, observed on-device).
…payment history

SPEC Part 6 ("nice UI") + M2 tasks 7-11, verified end-to-end on a
devnet: profile create, add contact by id, request/accept,
established contacts, send 0.01 DASH with txid in sender history,
received payments on the recipient's side across relaunches.

FFI (rs-platform-wallet-ffi):
- dashpay_sync.rs: 7 platform_wallet_manager_dashpay_sync_* symbols
  (start/stop/sync_now/is_syncing/is_running/interval get+set);
  sync_now runs via block_on_worker (8MB worker — GroveDB proof
  recursion overflows the caller thread's stack).
- dashpay_payment.rs: managed_identity_get_dashpay_payments getter.
- Persister callback arity 8→10: payment_channel_broken +
  contact-request rejection tombstones now cross the boundary.

Swift SDK:
- PersistentDashpayPayment model + persistence bridge;
  PersistentDashpayContactRequest gains rejection fields;
  PersistentIdentity payment relationship.
- PlatformWalletManagerDashPaySync: start/stop/refresh +
  @published dashPaySyncIsSyncing (1 Hz poll, sibling convention).
- Keychain unlock hook in loadFromPersistor: re-attaches the wallet
  seed via attach_wallet_seed so rehydrated wallets can sign.

SwiftExampleApp:
- New DashPay root tab (Views/DashPay/, 7 views): identity picker
  with @AppStorage persistence, profile header + editor, contacts +
  requests segments (incoming accept/reject, outgoing pending),
  add-contact (DPNS search + identity-id modes), contact detail
  (payments history, local alias/note/hide), send sheet. All §6.4
  interaction states; dashpay.* accessibility ids throughout.
- Contacts consolidated into the DashPay tab: legacy FriendsView
  (917 lines) deleted; IdentityDetailView's DashPay section now
  deep-links to the tab with the identity pre-selected (root tab
  selection moved to AppUIState). SendDashPayPaymentSheet +
  DashPayContact moved to Views/DashPay/.
- AddContactView guards partial base58 input (<32-byte decode
  crashed the FFI identifier precondition).

Tests: DashPayPersistenceTests (15 — persister bridge, tombstone
rotation-survival, payments), DashPayTabUITests (smoke).
Marks M2 + the receiver-side payment path as live-verified
(2026-06-12, devnet): account registrations now persisted, incoming
payments recorded live + reconciled after restore. Notes the drive
query-absence behaviour (equality without order-by proves absence)
referenced from the rs-sdk fix.
…detail

Contacts live in the DashPay tab now — the redirect row added during
the consolidation was an extra menu item with no unique function.
The identity screen keeps only identity-owned concerns (keys, DPNS,
balance, profile).
Three placement fixes from UI review:

- Sync page gains a "DashPay Sync Status" section (spinner while a
  pass is in flight, relative last-sync stamp from the FFI,
  Recurring/Stopped state, Sync Now) — the recurring DashPay loop
  was previously invisible there.
- DashPay tab shows "Received from contacts" under the profile
  header: the active identity's DashpayReceivingFunds balances,
  read from the same lock-free account-balance call the wallet
  list uses.
- Wallets account list hides the DashPay friendship accounts
  (tags 12/13): per-contact protocol plumbing that would bloat the
  list as contacts grow, and external accounts watch the contact's
  addresses (not our funds). Totals are unaffected — receiving
  funds already roll into Core Balance (verified live:
  9.39698657 = BIP44 9.37698657 + 0.02 received); the Storage
  Explorer still lists the raw rows.

Verified on-sim: sync section shows "Last sync: 5 secs /
Recurring"; DashPay tab shows 0.02000000 DASH received; no DashPay
rows remain in the Wallets account list.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Nitpick comments (1)
packages/swift-sdk/SwiftTests/SwiftDashSDKTests/DashPayPersistenceTests.swift (1)

421-465: ⚡ Quick win

Also assert the payment rows roll back in this atomicity test.

The doc comment says a mid-round persistDashpayPayments write must ride the open changeset and roll back with it, but the test only checks PersistentDashpayContactRequest. If payment persistence starts auto-saving again, this still passes. Add a PersistentDashpayPayment fetch before and after endChangeset(..., success: false) so the regression is pinned end-to-end.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@packages/swift-sdk/SwiftTests/SwiftDashSDKTests/DashPayPersistenceTests.swift`
around lines 421 - 465, In testPaymentRefreshDoesNotCommitAnOpenChangesetRound,
add assertions that verify the payment row staged by persistDashpayPayments is
not visible mid-round and is rolled back after endChangeset(..., success:
false); specifically, call the existing payment-fetch helper (or add/rename a
fetch function for PersistentDashpayPayment rows) to assert count == 0
immediately after the mid-round persist and again after
handler.endChangeset(..., success: false), mirroring the contact-row assertions
so the test verifies payment atomicity as well as contact atomicity.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- Around line 1919-1925: persistDashpayPayments is swallowing failures from
backgroundContext.save() via try?, which can silently drop payment-history
updates; change the save to propagate or log errors instead of ignoring them:
replace the try? backgroundContext.save() with a throwing or do/catch path
inside persistDashpayPayments that captures the thrown error from
backgroundContext.save(), records telemetry/logging (or rethrows to the caller)
with context (e.g., include which payment batch or wallet ID), and preserve the
existing inChangeset check (if !self.inChangeset) so the save still only runs
when appropriate; update callers or function signature as needed to handle
propagated errors or ensure telemetry is emitted in the catch.

In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/DashPayTabView.swift`:
- Around line 35-40: The optimisticSentIds and ownProfile state are
identity-scoped but currently persist across identity switches; update the
activeIdentity handling (the Task that observes activeIdentity) to reset
identity-scoped UI state at the start of the task: clear optimisticSentIds and
set ownProfile to nil (or otherwise remove cached profile) before loading;
alternatively refactor optimisticSentIds and ownProfile to be keyed by owner
identity (e.g., a dictionary keyed by activeIdentity.id) and read/write via that
key, and ensure loadOwnProfileFromCache() does not retain the previous profile
on read failure for the new identity but returns nil so the UI doesn’t show the
old identity’s data.

In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/IdentityDetailView.swift`:
- Around line 1235-1243: The avatar downloader currently accepts any parseable
URL, allowing non-HTTPS schemes; update fetchAvatarBytes to explicitly validate
the URL scheme and reject anything not exactly "https" before creating the
URLRequest, returning an error (or nil) for non-https inputs; locate
fetchAvatarBytes (and the analogous implementation referenced around lines
1390-1410) and add a guard that checks url.scheme?.lowercased() == "https" and
fails early with a clear error to prevent http or other schemes from being
fetched.

In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleAppUITests/DashPayTabUITests.swift`:
- Around line 92-97: Replace the immediate existence check on the toolbar
refresh button with a timed wait to avoid flakes: locate the `refresh` query
using `Identifier.refreshButton` in DashPayTabUITests (variable `refresh`) and
change the assertion to call `refresh.waitForExistence(timeout: ...)` instead of
checking `refresh.exists`, keeping the same failure message; choose a reasonable
timeout (e.g., 1–5s) consistent with other tests.

---

Nitpick comments:
In
`@packages/swift-sdk/SwiftTests/SwiftDashSDKTests/DashPayPersistenceTests.swift`:
- Around line 421-465: In testPaymentRefreshDoesNotCommitAnOpenChangesetRound,
add assertions that verify the payment row staged by persistDashpayPayments is
not visible mid-round and is rolled back after endChangeset(..., success:
false); specifically, call the existing payment-fetch helper (or add/rename a
fetch function for PersistentDashpayPayment rows) to assert count == 0
immediately after the mid-round persist and again after
handler.endChangeset(..., success: false), mirroring the contact-row assertions
so the test verifies payment atomicity as well as contact atomicity.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 9c0b4a7c-c449-41c7-bd16-7979ff30c777

📥 Commits

Reviewing files that changed from the base of the PR and between 9f770b8 and a51606d.

📒 Files selected for processing (42)
  • docs/dashpay/SPEC.md
  • packages/rs-platform-wallet-ffi/src/contact_persistence.rs
  • packages/rs-platform-wallet-ffi/src/dashpay_payment.rs
  • packages/rs-platform-wallet-ffi/src/dashpay_sync.rs
  • packages/rs-platform-wallet-ffi/src/lib.rs
  • packages/rs-platform-wallet-ffi/src/manager.rs
  • packages/rs-platform-wallet-ffi/src/persistence.rs
  • packages/rs-platform-wallet/src/changeset/core_bridge.rs
  • packages/rs-platform-wallet/src/error.rs
  • packages/rs-platform-wallet/src/manager/attach_seed.rs
  • packages/rs-platform-wallet/src/manager/dashpay_sync.rs
  • packages/rs-platform-wallet/src/manager/mod.rs
  • packages/rs-platform-wallet/src/wallet/identity/network/contacts.rs
  • packages/rs-platform-wallet/src/wallet/identity/network/dashpay_sync.rs
  • packages/rs-platform-wallet/src/wallet/identity/network/mod.rs
  • packages/rs-platform-wallet/src/wallet/identity/network/payments.rs
  • packages/rs-sdk-ffi/src/sdk.rs
  • packages/rs-sdk/src/platform/dashpay/contact_request_queries.rs
  • packages/swift-sdk/Sources/SwiftDashSDK/Persistence/DashModelContainer.swift
  • packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentDashpayContactRequest.swift
  • packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentDashpayPayment.swift
  • packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentIdentity.swift
  • packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/DashPayPayment.swift
  • packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedIdentity.swift
  • packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift
  • packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift
  • packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerDashPaySync.swift
  • packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift
  • packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/ContentView.swift
  • packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/SwiftExampleAppApp.swift
  • packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/AddContactView.swift
  • packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ContactDetailView.swift
  • packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ContactRequestsView.swift
  • packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ContactsView.swift
  • packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/DashPayContactMeta.swift
  • packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/DashPayProfileView.swift
  • packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/DashPayTabView.swift
  • packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/SendDashPayPaymentSheet.swift
  • packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/FriendsView.swift
  • packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/IdentityDetailView.swift
  • packages/swift-sdk/SwiftExampleApp/SwiftExampleAppUITests/DashPayTabUITests.swift
  • packages/swift-sdk/SwiftTests/SwiftDashSDKTests/DashPayPersistenceTests.swift
💤 Files with no reviewable changes (1)
  • packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/FriendsView.swift
✅ Files skipped from review due to trivial changes (3)
  • packages/rs-platform-wallet-ffi/src/lib.rs
  • packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/AddContactView.swift
  • docs/dashpay/SPEC.md
🚧 Files skipped from review as they are similar to previous changes (2)
  • packages/rs-platform-wallet/src/wallet/identity/network/mod.rs
  • packages/rs-platform-wallet/src/manager/mod.rs

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/DashPayTabView.swift (1)

23-26: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Scope the persisted active-identity key by network.

dashpay.activeIdentityId is shared across every network, so selecting an identity on testnet/devnet overwrites the remembered choice for mainnet too. When the user switches back, activeIdentity falls back to the first eligible identity instead of restoring the last selection on that network.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/DashPayTabView.swift`
around lines 23 - 26, The persisted AppStorage key stored in DashPayTabView
(`@AppStorage("dashpay.activeIdentityId") private var storedIdentityId`) is
global across networks; change it to be network-scoped by deriving the key from
the current network identifier (e.g., include network.rawValue or chainId) so
each network has its own stored key. Update DashPayTabView to compute the
AppStorage key at runtime (or use a computed property / wrapper that returns
"dashpay.activeIdentityId.\(networkId)") using the view’s network/environment
value and ensure storedIdentityId is read/written through that network-scoped
key so switching networks preserves separate selections.
♻️ Duplicate comments (1)
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/DashPayTabView.swift (1)

169-177: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Reset identity-scoped state before loading the next identity.

optimisticSentIds and ownProfile still survive an identity switch, and loadOwnProfileFromCache() explicitly keeps the previous profile on a read failure. That can render identity A's pending-request overlay or profile header under identity B until the cache catches up. Clear those fields at the start of the .task(id:) block, and don't retain the previous ownProfile in the failure path for the new identity.

Also applies to: 420-433

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/DashPayTabView.swift`
around lines 169 - 177, The task block keyed by .task(id:
activeIdentity?.identityId) is not resetting identity-scoped state: clear
optimisticSentIds and ownProfile immediately at the top of that task before
calling loadOwnProfileFromCache() and walletManager.dashPaySyncNow(); and update
loadOwnProfileFromCache() so that on a cache read failure for the new identity
it does not retain the previous ownProfile (set ownProfile to nil or replace
with an empty/default value) instead of keeping the old profile. Ensure the
reset refers to the existing properties optimisticSentIds and ownProfile and the
loadOwnProfileFromCache() function so the UI doesn't show the previous identity
while the new identity's cache is loaded.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AccountListView.swift`:
- Around line 35-42: The visible-empty-state logic is still checking the raw
accounts collection instead of the filtered/sorted list, causing the UI to hide
the "No Accounts" state when only accountType 12/13 are present; update the
empty-state checks to use orderedAccounts.isEmpty (or introduce a
visibleAccounts computed collection that filters out accountType 12/13 and reuse
it everywhere) and replace any usages of accounts.isEmpty / !accounts.isEmpty in
AccountListView with checks against that filtered collection so the UI matches
the displayed list (keep AccountListView.sortKey as the sorting helper).

---

Outside diff comments:
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/DashPayTabView.swift`:
- Around line 23-26: The persisted AppStorage key stored in DashPayTabView
(`@AppStorage("dashpay.activeIdentityId") private var storedIdentityId`) is
global across networks; change it to be network-scoped by deriving the key from
the current network identifier (e.g., include network.rawValue or chainId) so
each network has its own stored key. Update DashPayTabView to compute the
AppStorage key at runtime (or use a computed property / wrapper that returns
"dashpay.activeIdentityId.\(networkId)") using the view’s network/environment
value and ensure storedIdentityId is read/written through that network-scoped
key so switching networks preserves separate selections.

---

Duplicate comments:
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/DashPayTabView.swift`:
- Around line 169-177: The task block keyed by .task(id:
activeIdentity?.identityId) is not resetting identity-scoped state: clear
optimisticSentIds and ownProfile immediately at the top of that task before
calling loadOwnProfileFromCache() and walletManager.dashPaySyncNow(); and update
loadOwnProfileFromCache() so that on a cache read failure for the new identity
it does not retain the previous ownProfile (set ownProfile to nil or replace
with an empty/default value) instead of keeping the old profile. Ensure the
reset refers to the existing properties optimisticSentIds and ownProfile and the
loadOwnProfileFromCache() function so the UI doesn't show the previous identity
while the new identity's cache is loaded.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 5cf1916b-35bc-47ca-bb9d-48b3d9493945

📥 Commits

Reviewing files that changed from the base of the PR and between a51606d and a24bb43.

📒 Files selected for processing (4)
  • packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AccountListView.swift
  • packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CoreContentView.swift
  • packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/DashPayTabView.swift
  • packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/IdentityDetailView.swift
💤 Files with no reviewable changes (1)
  • packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/IdentityDetailView.swift

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

All 8 prior findings against 9f770b8 remain STILL VALID at a51606d — verified directly against the worktree (V001 unchanged, record_rejected_contact_request still omits removed_incoming, no reader for rejected_contact_requests, transient identity fetch still permanently breaks channels, purpose_mismatch still sticky, established-contact ingest still skips superseding requests, dashpay_sync cleanup still clobbers cancel token unconditionally, select_recipient_key_index still ignores disabled_at). The M2 delta also introduced one new blocker: Swift wallet deletion does not pre-delete the newly added PersistentDashpayPayment children whose owner inverse is non-optional, mirroring the contact-request pattern that the surrounding comment explicitly calls out as fatal. One FFI suggestion is worth flagging: the new DashpayPaymentFFI derives Copy despite owning two *mut c_char allocations reclaimed by dashpay_payment_array_free. Overflow: 1 valid suggestion dropped (register_external_contact_account persist outside write lock — conf 0.55).

🔴 2 blocking | 🟡 2 suggestion(s)

2 additional finding(s) omitted (not in diff).

6 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.

🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs:206-222: rejected_contact_requests is written but never read — tombstones lost across restart
  Verified at HEAD: managed_identity_from_entry still hard-codes rejected_contact_requests: Default::default() at line 214. The writer at contacts.rs:240 (INSERT INTO rejected_contact_requests) and migration row at V001:203 exist, but there is no load_state reader for the new table and apply_changeset only handles live deltas — restored state is always empty. The recurring DashPay loop's tombstone-skip at network/contact_requests.rs:396-404 never fires after a restart, so a rejected contact request is resurrected on the first sweep. Add a per-wallet load_state for rejected_contact_requests and route its output into the ContactChangeSet.rejected synthesized during rehydration.

In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:2980-2996: Wallet deletion omits new DashPay payment children before deleting identities — same fatal pattern as contact requests
  Verified: PersistentDashpayPayment.owner is declared as non-optional (`public var owner: PersistentIdentity`), and the new cascade relationship was added on PersistentIdentity.dashpayPayments in the M2 delta. The PHASE 1 pre-delete loop at lines 2986-2996 iterates dpnsNames, dashpayProfile, and contactRequests — but not dashpayPayments. The surrounding comment (lines 2962-2978) explicitly states this phase exists because SwiftData fatals during save() when it must null out a non-optional inverse on a child processed in the same delete batch. dashpayPayments has the exact same shape as contactRequests, so a wallet with persisted DashPay payments can crash or fail to wipe cleanly when deleted. Add a `for payment in Array(identity.dashpayPayments) { backgroundContext.delete(payment) }` loop alongside the existing pre-deletion.

In `packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs:245-261: select_recipient_key_index returns disabled keys
  Verified at HEAD lines 245-261: the selector iterates recipient_identity.public_keys() and returns the first DECRYPTION (then ENCRYPTION) ECDSA_SECP256K1 key with no disabled_at check. validate_contact_request in crypto/validation.rs does gate on disabled keys, so a recipient with a disabled preferred DECRYPTION key gets returned anyway and the broadcast fails downstream with an opaque error instead of falling through to a usable ENCRYPTION key on the same identity. Beyond reliability, on the send-side this also means the wallet could encrypt the DIP-15 compact xpub to a revoked key whose private half may be compromised. Add `&& k.disabled_at().is_none()` to both branches so selection matches validation.

In `packages/rs-platform-wallet-ffi/src/dashpay_payment.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/dashpay_payment.rs:89-108: DashpayPaymentFFI derives Clone, Copy despite owning *mut c_char strings reclaimed by dashpay_payment_array_free
  Verified at HEAD lines 89-108: DashpayPaymentFFI carries two heap-owned C strings (txid, memo — produced via CString::into_raw in cstring_or_null and reclaimed via CString::from_raw in dashpay_payment_array_free), yet the struct is `#[derive(Debug, Clone, Copy)]`. With Copy the compiler will silently shallow-duplicate the struct on any by-value rebinding inside this crate, and a subsequent free walk on the array (or a stray from_raw on the duplicate) would double-free the txid/memo allocations across the FFI boundary. Today's call sites are sound — the struct is built once, moved into Vec → Box<[T]> → Box::into_raw, and reclaimed exactly once — but the Copy derive removes the borrow-checker guardrail that normally prevents this class of bug at refactor time. Sibling FFI types in this crate that own heap pointers (ContactRequestFFI, WalletChangeSetFFI) deliberately omit Copy for exactly this reason. Drop Copy (and Clone if unneeded) on this struct. The cross-boundary contract is unchanged — Swift consumes by raw pointer.

Comment thread packages/rs-platform-wallet-ffi/src/dashpay_payment.rs
Conflict: identity_handle.rs — both sides appended a test module
(ours: ecdh_key_derivation_tests; upstream: master-derive tests from
the rescan fix). Kept both; 221/221 platform-wallet lib tests green
on the merged tree.

Also folds in a build fix the merged tree needs: upstream
CreateIdentityView's funding-source footer (string concatenation
with an embedded ternary) exceeds the Swift type-checker budget on
Xcode here — hoisted into a static helper, no copy change.
@shumkov shumkov changed the title fix(platform-wallet)!: DashPay sync correctness, consensus entropy fix, and DIP-15 mobile interop (M1) fix(platform-wallet)!: dashpay sync correctness, mobile interop, payments + DashPay tab (M1+M2) Jun 12, 2026
shumkov added 3 commits June 12, 2026 22:07
The explorer-coverage CI guard caught the M2 model addition: every
SwiftData model needs an explorer row + list view + detail view.
Adds the "DashPay Payments" section (network-scoped count, newest
first, full-column read-only detail), mirroring the contact-request
views. check-storage-explorer.sh: 28/28 covered.
…3, M3)

Send side:
- contact requests now carry the DIP-15 masked accountReference
  instead of a hardcoded 0: (version << 28) | (ASK28 ^ account).
  With the contract's unique index (ownerId, toUserId,
  accountReference), the constant 0 meant a superseding request
  after key rotation could never broadcast (duplicate-unique
  rejection) — the version bump is what makes re-keying possible.
- Re-sending to a recipient with a tracked prior request unmasks the
  prior version and bumps it (saturating at the 4-bit max with a
  warning).

Crypto helper fixes (research/06 §3 found both axes wrong):
- HMAC input is now the 69-byte DIP-15 compact xpub (both reference
  clients agree), not the 107-byte DIP-14 encode().
- ASK28 extraction matches iOS dash-shared-core: digest bytes
  [28..32] big-endian >> 4. The reference clients disagree with each
  other here (Android: bytes [0..4] LE) — recipients must disregard
  the field per DIP-15, so the binding consumer is our own
  round-trip; we follow the Rust reference implementation and flag
  the divergence for a DIP clarification.
- New unmask_account_reference recovers (version, account) for the
  sender.

Receive side (DIP-15 "sender rotated their addresses"):
- Sync ingest dedups by (sender, accountReference) instead of bare
  sender id: a known sender with a NEW reference is a rotation
  request and passes the guard (the old guard silently dropped it).
- apply_rotated_incoming_request supersedes the tracked request
  (last-write-wins per pair; simultaneous multi-account rides
  acceptedAccounts later), clears payment_channel_broken — the
  recovery the flag's contract promises — and the sync pass tears
  down the stale external account so the build sweep re-registers
  it from the rotated xpub.

Tests: ASK28 byte-order pin (fails on the old head-of-digest read),
mask/unmask round-trip across version/account ranges, rotation
re-key + broken-flag clear + pending-replace + stranger no-op.
223/223 lib + 9/9 workflow green.
Shared-secret-only callback on the existing host-signer table; the
identity private key never crosses the ABI. EcdhProvider routing
stays internal to platform-wallet so M4's implementation lands
without wallet-API churn. One hook covers both send-side and
decrypt-side ECDH.

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Re-verified all 10 prior findings against worktree HEAD aabc21e; every one is STILL VALID — none of the three Swift example-app commits or the v3.1-dev merge touched the dashpay-correctness Rust/Swift hotspots. Carrying forward 7 blockers (append-only V001 violation, rejected-request persistence asymmetry, missing tombstone reader, transient-as-permanent channel breakage, sticky purpose_mismatch, unreachable broken-channel recovery, SwiftData wallet-deletion miss on the new payments cascade) and 3 suggestions (sync stop/start cleanup race, send-side disabled-key selection, Copy on FFI-owned C-strings). REQUEST_CHANGES.

🔴 7 blocking

3 additional finding(s) omitted (not in diff).

3 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.

🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rs:109-131: record_rejected_contact_request drops incoming in memory but never persists the deletion
  Verified at HEAD lines 109-131: line 116 calls `self.incoming_contact_requests.remove(sender_id)` but the returned `ContactChangeSet` (lines 127-129) only populates `cs.rejected`. The unified contacts-table writer in `rs-platform-wallet-storage/src/sqlite/schema/contacts.rs` only DELETEs incoming rows when `cs.removed_incoming` is non-empty, so the persisted `state='received'` row with its stale `incoming_request` blob stays in SQLite. Once `persister.load()` is wired up, the contacts reader re-buckets that row as an incoming request and `apply_changeset` re-inserts it — the explicitly-rejected request reappears in the UI. The in-memory mutation and the persisted delta are internally inconsistent. Emit a matching `ReceivedContactRequestKey { owner_id, sender_id: *sender_id }` into `cs.removed_incoming` alongside `cs.rejected`.

In `packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs:206-222: rejected_contact_requests tombstones are written but never restored on rehydration
  Verified at HEAD: `managed_identity_from_entry` still hard-codes `rejected_contact_requests: Default::default()` at line 214. The writer at `contacts.rs:240` (`INSERT INTO rejected_contact_requests`) and the migration row at `V001:203` exist, but no `load_state` reader for the new table exists, and `apply_changeset` only handles live deltas — restored state is always empty. The recurring DashPay loop's tombstone-skip at `network/contact_requests.rs:396-404` therefore never fires after a restart, so a rejected contact request is resurrected on the first sweep and surfaces to the user again. Security framing: the on-platform document is immutable, so the local tombstone is the ONLY thing that suppresses a spammer's repeated contact request — a wipe-on-restart defeats the user's explicit reject. Add a per-wallet `load_state` reader for `rejected_contact_requests` and route its output into the `ContactChangeSet.rejected` synthesized during rehydration.

In `packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs:711-729: Transient DAPI failures inside register_external_contact_account are classified as permanent
  Verified at HEAD lines 711-729: `build_contact_accounts` treats ANY error from `register_external_contact_account` as permanent and calls `mark_contact_channel_broken`. `register_external_contact_account` performs a fresh `Identity::fetch` internally and wraps DAPI/network failures as `PlatformWalletError::InvalidIdentityData`. A single transient DAPI hiccup therefore permanently disables the payment channel; subsequent sweeps skip the contact via the `payment_channel_broken` filter, and recovery only fires if a superseding contactRequest arrives — but the established-contact ingest skip at line 389 makes that path unreachable. Combined, a transient network event bricks a channel forever, and a malicious or unreliable DAPI endpoint becomes a persistent availability attack against payments to a specific contact. Fix by either passing the pre-fetched identity into registration or scoping permanent-broken classification to genuinely non-recoverable failures (decrypt/decode, missing-key, key-type mismatch).
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs:383-404: Established-contact ingest skip makes payment_channel_broken recovery unreachable
  Verified at HEAD lines 383-404: the received-side ingest drops every doc whose sender is already in `established_contacts` (line 389) BEFORE consulting `accountReference`. `EstablishedContact::reestablish_preserving_metadata` exists precisely to clear `payment_channel_broken` when a fresh request flows in, and `collect_account_build_candidates` documents the recovery contract ("never retry a permanently-broken channel — wait for a superseding request which clears the flag on re-establish"). But no path reaches `reestablish_preserving_metadata` for an already-established sender from the sync sweep — `add_incoming_contact_request` is only called for new senders here, and the send-side guard at `state/managed_identity/contact_requests.rs:46-48` also returns early for established contacts. Once `payment_channel_broken` is set, it stays set forever. Either detect a new `accountReference` for the same sender and route through the reestablish path, or change the broken-channel policy to permit controlled retry.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs:245-261: select_recipient_key_index returns disabled (revoked) keys — DIP-15 compact xpub encrypted to a key whose private half may be compromised
  Verified at HEAD lines 245-261: the selector iterates `recipient_identity.public_keys()` and returns the first DECRYPTION (then ENCRYPTION) ECDSA_SECP256K1 key with no `disabled_at` check. `validate_contact_request` in `crypto/validation.rs` does gate on `disabled_at`, so the asymmetry creates both a reliability bug (an opaque downstream broadcast failure instead of falling through to a usable key) AND a real confidentiality exposure: on send, the wallet encrypts the 69-byte DIP-15 compact xpub (fingerprint‖chaincode‖pubkey — combined with `accountReference` lets the holder derive every receiving address on that account) to a key the recipient has explicitly revoked. Identity-key revocation is the on-platform mechanism for declaring "the private half of this key may be compromised". Add `&& k.disabled_at().is_none()` to both branches so selection matches validation. (Promoted from suggestion to blocking on the security-auditor confidentiality analysis.)

In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:3018-3035: Wallet deletion PHASE 1 omits PersistentDashpayPayment children — same fatal pattern as contactRequests
  Verified at HEAD: PHASE 1 (lines 3024-3034) iterates `dpnsNames`, `dashpayProfile`, and `contactRequests` but NOT `dashpayPayments`. `PersistentDashpayPayment.owner` is non-optional (`PersistentDashpayPayment.swift:98`), and `PersistentIdentity.dashpayPayments` is the cascading inverse added in this PR. The surrounding comment (lines 3018-3023) explicitly states this phase exists because SwiftData fatals during `save()` when it must null out a non-optional inverse on a child processed in the same batch — exactly the shape of the new payments relationship. A wallet with persisted DashPay payment history will hit the SwiftData fatal at PHASE 2 (`save()` after `delete(identity)`), aborting before the wallet row is removed. The user's belief that they wiped DashPay data is wrong, and plaintext memo + counterparty id + amount + txid rows remain on disk. Add a `for payment in Array(identity.dashpayPayments) { backgroundContext.delete(payment) }` loop alongside the existing pre-deletions.

In `packages/rs-platform-wallet/src/manager/dashpay_sync.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/manager/dashpay_sync.rs:192-235: DashPaySyncManager thread cleanup unconditionally clears the cancel token — stop/start race enables use-after-free across FFI
  Verified at HEAD lines 192-235: the spawned thread's cleanup at lines 230-232 writes `*guard = None` on loop exit regardless of which token the slot currently holds. If `stop()` cancels the old token and `start()` installs a fresh token before the old thread reaches cleanup, the old thread clears the NEW token — `background_cancel` is empty while a fresh sync thread is still running. `stop()`/`quiesce()` then become a no-op against that running thread. In this PR's Swift integration the persister and DashPay event callbacks close over an UnsafePointer<Context> allocated on the Swift side; calling `dashpay_sync_manager_destroy` (or the wallet manager's drop) after the visible token was cleared frees that context while the surviving thread continues invoking the callbacks against the freed pointer — a concrete use-after-free crossing the C ABI, reachable through normal start/stop/destroy controls (toggling tabs, login/logout) and widened by attacker-influenced network timing. Capture the spawned token and only clear the slot if it still holds the same token (`Arc::ptr_eq`), mirroring `ShieldedSyncManager`'s generation guard. (Upgraded from suggestion to blocking based on the security-auditor and codex-security cross-checks of the destroy/UAF path.)

Comment thread packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs Outdated
Comment thread packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs Outdated
Comment thread packages/rs-platform-wallet/src/manager/dashpay_sync.rs
shumkov added 2 commits June 12, 2026 22:34
…earch

Decisive: no reference client (DashSync-iOS, dashj, dash-shared-core)
ever implemented contactInfo — our implementation sets the de-facto
convention. Adopts: DIP-15 child derivation (root/65536'+65537'/idx'),
AES-256-ECB encToUserId, IV-prepended AES-256-CBC privateData, CBOR
array [aliasName, note, displayHidden] per the deployed schema (which
contradicts DIP-15 prose — table included), ≥2-contacts publish gate.
… part 1)

The crypto core for DashPay contactInfo documents, following the
conventions recorded in docs/dashpay/research/07 (no reference client
ever implemented this doc type — this sets the de-facto wire format):

- platform-encryption: AES-256-ECB encrypt/decrypt for the 32-byte
  encToUserId (two raw blocks, no IV/padding — DIP-15's own ECB
  soundness argument: the plaintext is a SHA-256 output and the key
  is single-purpose), plus IV-prepended AES-256-CBC helpers for
  privateData. Tests pin the ECB property (identical blocks encrypt
  identically) so a CBC-with-zero-IV regression can't slip in.

- platform-wallet crypto/contact_info.rs: DIP-15 key derivation
  (rootEncryptionKey / 65536' / index' for encToUserId,
  / 65537' / index' for privateData — hardened children of the
  identity's registered ENCRYPTION key path), CBOR codec for the
  deployed schema's array shape [aliasName, note, displayHidden]
  with a 4th ignored padding element lifting tiny payloads to the
  schema's 48-byte ciphertext floor.

Tests: key-derivation determinism + domain separation, CBOR
round-trip incl. all-absent payload, full derive→encrypt→decrypt
round-trip with schema bounds check.

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Reconciliation against HEAD 440ffca: prior finding #6 (established-contact ingest skip) is FIXED by the new rotation path. Nine prior findings remain STILL VALID and the new G3 delta introduces two additional blockers — the send-side rotation-version lookup ignores established_contacts (forcing version=0 collisions after auto-establishment) and the receive-side rotation handler replays immutable historical requests as fresh rotations on every sweep, churning the external account. Total: 10 in-scope blockers. Overflow: 1 suggestion (DashpayPaymentFFI Copy) dropped due to budget.

🔴 5 blocking

2 additional finding(s) omitted (not in diff).

5 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.

🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs:165-201: Send-side rotation version lookup ignores established_contacts — re-send after auto-establishment reuses version=0 and collides on the unique index
  The new G3 rotation logic computes `previous_version` only from `managed.sent_contact_requests.get(recipient_identity_id)` (line 171). But establishment (`add_incoming_contact_request` line 175 and `apply_established_contact` line 372 in state/managed_identity/contact_requests.rs) explicitly removes the entry from `sent_contact_requests` and parks the prior outgoing request on `EstablishedContact.outgoing_request`. Once the reciprocal arrives and a sweep auto-establishes the pair, the next `send_contact_request` to that recipient sees `previous_version = None` and falls back to `version = 0`. With deterministic xpub/ECDH for the same (sender, recipient) and unchanged `account_index`, the PRF reproduces the same masked `account_reference` as the original sent request. The contract's unique index `($ownerId, toUserId, accountReference)` rejects the broadcast — the exact failure mode G3 was added to prevent. Fall back to `established_contacts[recipient].outgoing_request` (taking the max of both versions if both are present).
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs:441-478: Historical contactRequest documents replay as fresh rotations every sync sweep
  The rotation guard at line 451 only compares the incoming reference against the currently tracked reference (incoming map or established contact). contactRequest documents are immutable, and `fetch_received_contact_requests(identity_id, None)` (line 370) is unfiltered, so every sweep returns both the original v=0 and any rotated v=N documents. Within a single sweep, ingesting v=0 against an already-tracked v=N flips the established contact back to v=0 and queues a teardown (lines 472-478, then 517-528), and the next document in the same iteration flips it forward again. Across sweeps the same churn replays — the external account is torn down and rebuilt on every cycle, generating wasted DAPI traffic. Worse, if the freshest document falls outside the eventual paginated window (post-M3 growth), the contact can regress to the stale xpub. Compare by `created_at`/version monotonicity, not bare reference inequality: only apply rotation when the incoming request strictly supersedes the tracked one.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs:292-308: select_recipient_key_index returns disabled (revoked) keys — DIP-15 compact xpub encrypted to a key whose private half may be compromised
  Verified at HEAD lines 292-308: the selector iterates `recipient_identity.public_keys()` and returns the first DECRYPTION (then ENCRYPTION) ECDSA_SECP256K1 key with no `disabled_at` check. `validate_contact_request` does gate on `disabled_at`, so the send/receive interop rules are asymmetric. On send, the 69-byte DIP-15 compact xpub (fingerprint‖chaincode‖pubkey — combined with `accountReference` lets the holder derive every receiving address on that account) is encrypted to a key the recipient has explicitly revoked. Identity-key revocation is the on-platform mechanism for declaring 'this key's private half may be compromised'. Add `&& k.disabled_at().is_none()` to both branches so selection matches validation.

In `packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs:206-222: rejected_contact_requests tombstones are written but never restored on rehydration
  `managed_identity_from_entry` still hard-codes `rejected_contact_requests: Default::default()` at line 214. The writer at `sqlite/schema/contacts.rs:240` (INSERT INTO rejected_contact_requests) and the V001 row exist, but there is no `load_state` reader for the new table, and `apply_changeset` only handles live deltas — restored state is always empty after restart. The recurring DashPay loop's tombstone-skip at `network/contact_requests.rs:457` therefore never fires after restart, so a rejected request is resurrected on the first sweep. Because the on-platform document is immutable, wiping the tombstone on restart defeats the user's explicit reject. Add a per-wallet `load_state` reader for `rejected_contact_requests` and route its output into the rehydration changeset / ManagedIdentity field.

In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:3024-3034: Wallet deletion PHASE 1 omits PersistentDashpayPayment children — same fatal pattern as contactRequests
  PHASE 1 iterates `dpnsNames`, `dashpayProfile`, and `contactRequests` but NOT `dashpayPayments`. `PersistentDashpayPayment.owner` is non-optional and `PersistentIdentity.dashpayPayments` is the cascading inverse added in this PR. The surrounding comment (lines 3008-3023) explicitly documents that this phase exists because SwiftData fatals during `save()` when it must null out a non-optional inverse on a child processed in the same batch — exactly the shape of the new payments relationship. A wallet with persisted DashPay payment history will hit the SwiftData fatal at PHASE 2 `save()`, aborting before the wallet row is removed. The user believes their data was wiped; plaintext memo + counterparty id + amount + txid rows remain on disk. Pre-delete `identity.dashpayPayments` alongside the other children.

shumkov and others added 14 commits June 23, 2026 14:09
…error path

`dash_sdk_sign_with_mnemonic_resolver_and_path` derived `master` and `derived`
ExtendedPrivKeys and only scrubbed them on the success path via hand-placed
`non_secure_erase()`. The `derive_priv` error path returned without wiping
`master`, and a panic anywhere between derivation and signing left both
scalars in memory unscrubbed.

Wrap both keys in a `WipingXprv` RAII guard whose Drop calls
`non_secure_erase()`, so the scalars are scrubbed on every exit — success,
error return, and unwind. Mirrors the existing `WipingXprv` guard in
rs-sdk-ffi/src/mnemonic_resolver_core_signer.rs.

No regression test: Drop-based memory scrubbing is not observable from safe
Rust, and forcing `derive_priv` to fail mid-derivation isn't deterministic —
this is the untestable-path exception. Correctness is by construction (guard
covers all exits).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… verify_seed_binds

Removes the resident-seed workaround the DashPay seedless posture exists to
eliminate. `attach_wallet_seed` re-derived a signing wallet from the seed and
grafted the seed-bearing WalletType onto the loaded watch-only wallet via
`std::mem::swap`, reintroducing a resident seed on every Keychain unlock — the
exact thing the external-signer model removes.

Its only load-bearing behaviour was the dual-gate wrong-seed check (id-match OR
BIP44-0 xpub-match) that refused to graft a mismatched seed. That detection is
preserved without ever holding a seed, in the SAME change (no window where a
mis-mapped Keychain slot could sign for the wrong wallet):

- New `PlatformWallet::verify_seed_binds<C: ContactCryptoProvider>`: reads the
  wallet's persisted BIP44 account-0 xpub and the exact path it was rooted at
  (from the same `Account`, so they can't drift), derives the xpub at that path
  through the signer (`ContactCryptoProvider::receiving_xpub`, a plain
  derive-at-path), and compares. Mismatch -> `SeedMismatch`. The wallet stays
  watch-only; only the signer (which holds the seed) derives.
- New FFI `platform_wallet_verify_seed_binds_to_wallet(wallet_handle,
  core_signer_handle)`, mirroring the drain entry point: build the resolver
  provider, run the check on a worker thread. The host calls it at unlock
  alongside the deferred-crypto drain.
- `SeedMismatch` repurposed to the xpub-binding check (dropped the now-unused
  `derived_id` field; its only consumer was the deleted attach FFI).

Reuses the existing, already-tested `MnemonicResolverCoreSigner::verify_binds_to_xpub`
primitive via the provider seam rather than adding a redundant trait method.

Deleted: `manager/attach_seed.rs` (impl + 5 tests), the
`platform_wallet_manager_attach_wallet_seed_from_mnemonic` FFI export + its 4
marshalling tests, and all textual references (`git grep attach_wallet_seed` is
now empty across the Rust tree).

Tests: the deleted dual-gate happy/mismatch tests are replaced by
`verify_seed_binds_{accepts_matching,rejects_wrong}_signer`, exercised through
`SeedCryptoProvider`. The wrong-seed test is non-tautological: neutering the
`derived == expected` comparison turns it red (verified), the real comparison
turns it green. platform-wallet 292 + glue 112/26/5 green.

Swift unlock rework (call verify instead of attach, drain-on-unlock) follows in
the next commit; on-device testnet acceptance is the remaining env-gated step.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…d_binds landed

Status table gains the WipingXprv sibling-FFI fix, the RawContactInfoDoc
visibility fix, and the atomic attach_wallet_seed deletion + verify_seed_binds
wiring. Q2 banner step 4 MUST-FIX (atomic wrong-seed wiring) and §4.2 SHOULD-FIX
marked done (Rust); Swift unlock rework + KeychainSigner nil-swallow + on-device
testnet acceptance remain.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…seed graft

Reworks `unlockWalletFromKeychain` for the seedless posture and threads the
resolver-backed core signer through the remaining DashPay write paths, matching
the Rust FFI that replaced `attach_wallet_seed` with `verify_seed_binds`.

`unlockWalletFromKeychain(_ wallet:)` (was `(walletId:)`):
- No longer grafts a resident seed via the deleted
  `platform_wallet_manager_attach_wallet_seed_from_mnemonic`.
- Existence-only Keychain check (`hasMnemonic`) — the plaintext mnemonic is
  never pulled into Swift; a wallet with none stays watch-only.
- Wrong-seed gate: `platform_wallet_verify_seed_binds_to_wallet` derives the
  BIP44 account-0 xpub through a `MnemonicResolver` and compares it to the
  persisted one; a mis-mapped Keychain slot throws (logged-and-continued per
  wallet, so it can't sign for a wallet it doesn't own).
- Drains deferred contact-crypto in a detached background task
  (`platform_wallet_drain_pending_contact_crypto`) — it re-fetches/decrypts over
  the network, so it must not block the restore loop.

send / accept / contactInfo (`ManagedPlatformWallet`): each now builds a
`MnemonicResolver` and passes `coreSigner.handle` as the new `core_signer_handle`
arg the Rust FFI gained, keeping both the document signer and the resolver alive
across the call via `withExtendedLifetime((signer, coreSigner))` (the bare
`_ = signer` keepalive is insufficient for the vtable-callback lifetime).

Deleted the legacy `KeychainSigner.sign(...) -> Data?` nil-swallow (a
silent-failure path with zero callers repo-wide) and its now-empty `Signer`
protocol requirement; the protocol keeps only `canSign`. Signing happens solely
through `KeychainSigner.handle` on the FFI path.

Verified: regenerated the unified xcframework header (build_ios.sh --target sim)
and built SwiftExampleApp for arm64 simulator — BUILD SUCCEEDED, zero Swift
errors. On-device testnet acceptance is the remaining env-gated Q2 step.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… remains

Marks the spec banner steps 3/4 Swift work done (verify-binds + drain-on-unlock,
core_signer_handle threading, KeychainSigner.sign nil-swallow removed) and the
"Done when" criteria: grep-empty, cargo green, build_ios.sh + SwiftExampleApp
build clean — all met. The only remaining Q2 item is the env-gated testnet
on-device acceptance (happy + wrong-seed-rejected + cross-device contactInfo).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…erseded as done

The handoff's "environment-blocked" section is superseded: the Swift unlock
rework is landed (70aaf32) and an on-device smoke test (booted simulator,
fresh install + launch) reached the Sync Status screen with DashPay sync running
against real persisted state — the new seedless binary launches and runs with no
crash. The only open Q2 item is the funded testnet acceptance (interactive
manual QA: funded wallet, mis-mapped slot, second device, on-chain writes).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Drove the SwiftExampleApp on the booted simulator (idb UI automation) against the
funded devnet Test_devnet wallet (9 DASH; 3 identities with platform credits) and
validated the seed-elimination end-to-end on-chain:

- Seedless contactInfo publish — setDashPayContactInfo (the core_signer_handle-
  threaded Swift call) sealed + published an encrypted contactInfo doc via the
  Keychain resolver, both create (updated=false) and update (updated=true), to
  Platform/DAPI. No resident seed. This is the publish half of the cross-device
  contactInfo flow.
- Seedless payment — send_dashpay_payment signed the tx via the resolver; broadcast
  was blocked only by "SPV Client not started" (orthogonal env state, not a seed
  defect).

Remaining acceptance is manual/env-gated: wrong-seed-rejected at unlock (covered by
the verify_seed_binds unit test; on-chain trigger needs a destructive wipe+reimport),
cross-device contactInfo decrypt (needs a 2nd device), and the full clean-wipe→import
happy path with SPV started.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Drove two simulators (idb) on devnet paloma and validated the entire seedless
signing surface on-chain — every op via the Keychain resolver, no resident seed:

- Core payment + InstantLock (sim A -> sim B funding, 1 DASH).
- Identity registration (asset lock + IS/CL proofs + Platform) on sim B.
- DPNS name (eveqaseed1ess2026) + DashPay profile (Eve) publish.
- contactInfo publish (create + update) via setDashPayContactInfo.
- Contact request SEND (Alice->Eve) — DashpayReceivingFunds registered.
- Contact request ACCEPT (Eve->Alice) — reciprocal + DashpayReceivingFunds +
  DashpayExternalAccount; contact established (sim A shows Eve as contact #3).

The send/accept/contactInfo paths all exercise the new core_signer_handle Swift
wiring end-to-end. Remaining (covered elsewhere): wrong-seed-rejected at unlock
(unit-tested; on-chain trigger needs a destructive wipe+reimport) and same-owner
cross-device contactInfo decrypt (publish validated, decrypt unit-tested).

Also flags an unrelated DAPI bug: contact-profile chunk fetch fails with
"missing order by for range" (does not block contact establishment).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… orderBy)

Found during the two-simulator on-device acceptance: fetch_contact_profiles_chunk
(profile.rs:737) issues a `$ownerId In [chunk]` query (a range op) with empty
order_by_clauses, so DAPI rejects it with "missing order by for range error".
Contact profiles never render (establishment is unaffected). Filed as a P0 bug
with the exact site + fix (add an $ownerId ascending OrderClause). Unrelated to
seed elimination.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ts, key hygiene)

Post-review batch from the 6-lens read of the seed-elimination final unit
(verdicts: 5 ship-with-fixes, 1 ship; no blockers — wrong-seed gate + crypto
core independently endorsed sound). Applied the quick-win fixes:

- M1: delete the orphaned `MnemonicResolverCoreSigner::verify_binds_to_xpub` + its
  `WrongSeed` error variant + its tautology test. The live verify path
  (`PlatformWallet::verify_seed_binds`) compares via `receiving_xpub`→
  `extended_public_key`, never this method — it had zero production callers
  (flagged independently by the rust-quality and correctness reviewers).
- M2: add a `verify_seed_binds` test for the `get_bip44_account(0) == None`
  branch (→ `InvalidIdentityData`, fail-closed).
- M3: add FFI marshalling tests for `platform_wallet_verify_seed_binds_to_wallet`
  (null `core_signer_handle` → `ErrorNullPointer`; unknown `wallet_handle` →
  `NotFound`) — restoring the boundary coverage the removed attach FFI had.
- L1: `WipingSecretKey` RAII guard for the leaf `secp256k1::SecretKey` in
  `sign_ecdsa`/`public_key` so the scalar copy is scrubbed on panic/unwind too,
  not just the Ok path (security reviewer's panic-window finding).
- M4: drain log says "processed N" not "drained N" — the count includes
  channel-broken give-ups, so don't imply all succeeded.
- M5: `unlockWalletFromKeychain` catch distinguishes a wrong-seed binding
  (`SeedMismatch`→`.invalidParameter`, a security event) from a transient failure.
- docs/L: reword `ContactCryptoProvider::receiving_xpub` (generic derive-at-path);
  document the detached drain's handle-capture/no-op-on-destroy lifetime.

platform-wallet 293, glue 114, rs-sdk-ffi 298/2/34 green; xcframework rebuilt +
SwiftExampleApp arm64 BUILD SUCCEEDED.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ollow-ups

- Spec: the wrong-seed wiring note said it "reuses verify_binds_to_xpub via the
  receiving_xpub seam" — inaccurate. The comparison lives in verify_seed_binds
  (derives through receiving_xpub); the signer's verify_binds_to_xpub was unused
  and has been deleted. Corrected the note.
- TODO: mark §4.9 deletion, the Swift unlock rework, and cross-device on-device
  acceptance done; add the deferred multi-reviewer follow-ups (needs-unlock /
  verify-failed UI signal → fold into §4.7/§9-7; de-dup WipingXprv; restored-wallet
  verify test; remove the one-method Signer shim when the API narrows).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Drop the WipingXprv de-dup item — two documented 6-line guards are fine; the
  real fix is upstream ZeroizeOnDrop (now tracked under §4.2).
- Reframe the "restored-wallet verify test": on investigation it is NOT a
  verify_seed_binds test — load_from_persistor receives already-deserialized
  structs, so a platform-wallet test duplicates create-path coverage and can't
  catch a serde regression. The real account_xpub round-trip is the FFI persister
  decoding AccountSpecFFI.account_xpub_bytes; point the follow-up there.
- Mark §4.2 done (WipingXprv ported + WipingSecretKey added); fold the
  ZeroizeOnDrop upstream residual into it.
- Fix §4.8's now-stale reference to the deleted verify_binds_to_xpub → verify_seed_binds.
- Keep: needs-unlock/verify-failed UI signal, Signer-shim removal, ZeroizeOnDrop.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ore round-trip

The reviewer's "restored-wallet verify test" reframed to its correct layer. A
platform-wallet verify_seed_binds test would be a near-duplicate — load_from_persistor
receives already-deserialized structs, and verify reads account_xpub identically
however the account was built, so it can't regress from a serde bug. The real
byte round-trip is in the FFI persister.

`account_xpub_survives_persist_restore_round_trip` drives the exact production
chain: bincode encode_to_vec(account_xpub) (store) → AccountSpecFFI.account_xpub_bytes
→ slice_from_raw → decode_from_slice::<ExtendedPubKey> → Account::from_xpub (restore),
asserting the xpub the seed-binding gate compares against survives byte-for-byte.

Non-tautological: mismatching the store/restore bincode config (standard vs legacy)
fails the decode (verified red→green). glue 115 green.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… modules

Addresses the review comment that lib.rs had grown into spaghetti after the
DIP-15 work. Splits the 735-line lib.rs by concern, with tests moved next to
their code:

- ecdh.rs              — ECDH shared-secret derivation
- aes.rs               — AES-256-CBC primitives (shared by the encrypted fields)
- compact_xpub.rs      — 69-byte compact xpub + its encryption
- account_label.rs     — encryptedAccountLabel
- contact_info.rs      — contactInfo (encToUserId + privateData)
- account_reference.rs — masked accountReference
- error.rs             — CryptoError

lib.rs is now module declarations + `pub use` re-exports, so the public API is
unchanged (flat `platform_encryption::*`); every downstream caller
(platform-wallet, rs-sdk-ffi, platform-wallet-ffi) builds unchanged. Pure code
move — no behavior change; all 15 tests carried over and pass.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Cumulative + delta review at e74e4e2. Eight prior findings re-verify unchanged at HEAD; the rs-sdk-ffi xpub-length finding is fixed by deletion of the entire dash_sdk_dashpay_* surface. One new blocking finding from the latest delta: contact-profile chunk query omits the orderBy required for an $ownerId In range, breaking the new contact-profile sync at runtime (the PR has docs-filed the bug but not fixed the code). One new suggestion on autoAcceptProof FFI length bounds. Codex FFI's ABI-break claim is dropped as a false positive — the *_with_signer symbols are new entry points and there is no shipped stable C ABI. Action: REQUEST_CHANGES.

🔴 1 blocking | 🟡 9 suggestion(s)

10 additional finding(s)

blocking: Contact-profile chunk query omits orderBy required for $ownerId In

packages/rs-platform-wallet/src/wallet/identity/network/profile.rs (line 752)

fetch_contact_profiles_chunk builds a $ownerId In [chunk] query but leaves order_by_clauses: vec![] at line 763. In is a range operator and DAPI rejects the request with 'missing order by for range error' — every contact-profile chunk fails and is skipped, so the new contact-profile cache/UI never receives remote profiles. The PR's own commit 82c85220ff documents this as a known bug but has not fixed it. Since this PR ships the contact-profile sync feature that the new SwiftUI Requests tab depends on, the fix is a prerequisite, not a follow-up. Add an OrderClause { field: "$ownerId", ascending: true } to order_by_clauses (importing OrderClause from dash_sdk::drive::query).

suggestion: established_contact_get_note leaves *out_note undefined on early-return paths

packages/rs-platform-wallet-ffi/src/established_contact.rs (line 156)

Sibling established_contact_get_alias (103-117) writes *out_alias = std::ptr::null_mut() immediately after check_ptr!, establishing the module's C-ABI contract that out-params are defined-null on early returns. established_contact_get_note skips this pre-null write at line 160. Early returns from unwrap_option_or_return! (storage NotFound or contact.note == None) and unwrap_result_or_return! (CString::new failure on interior NUL — reachable from counterparty-controlled note content via established_contact_set_note) leave *out_note indeterminate across the ABI. A caller that doesn't pre-zero the slot can read a stale pointer or pass it to platform_wallet_string_free, risking invalid-free or double-free. One-line fix matches the sibling pattern exactly.

pub unsafe extern "C" fn established_contact_get_note(
    contact_handle: Handle,
    out_note: *mut *mut std::os::raw::c_char,
) -> PlatformWalletFFIResult {
    check_ptr!(out_note);
    *out_note = std::ptr::null_mut();

    let option =
        ESTABLISHED_CONTACT_STORAGE.with_item(contact_handle, |contact| contact.note.clone());
    let option = unwrap_option_or_return!(option);
    let note = unwrap_option_or_return!(option);
    let c_str = unwrap_result_or_return!(std::ffi::CString::new(note));
    unsafe { *out_note = c_str.into_raw() };
    PlatformWalletFFIResult::ok()
}
suggestion: Pending incoming-request rows fetch attacker-controlled avatar URLs before user consent

packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ContactRequestsView.swift (line 119)

sync_contact_profiles (profile.rs:642) includes managed.incoming_contact_requests.keys() in the per-owner fetch plan, populating the contact-profile cache for senders the victim has never accepted. IncomingRequestRow feeds cachedProfile(row.contactIdentityId)?.avatarUrl into DashPayAvatarView → SwiftUI AsyncImage, issuing an outbound HTTPS GET to the sender-controlled host on first render — leaking IP, online state, TLS/UA fingerprint, and per-victim URL-token receipt confirmation before any consent action. With PersistentDashpayContactProfile SwiftData persistence the channel survives restart; is_valid_avatar_url only validates scheme/length. Ignore mutes only after first render. Either suppress remote avatars Swift-side for unaccepted senders (initials-only variant for the Requests tab) or strip avatarUrl from the Rust→Swift projection until the sender becomes an established contact.

suggestion: DashPay profile reader trio leaves *out_profile / *out_has_profile undefined on early-return paths

packages/rs-platform-wallet-ffi/src/dashpay_profile.rs (line 90)

managed_identity_get_dashpay_profile, platform_wallet_get_dashpay_profile, and platform_wallet_get_contact_profile only assign *out_profile / *out_has_profile inside the Some/None match arms. After check_ptr!, early returns from unwrap_result_or_return!(read_identifier(..)) and unwrap_option_or_return! (handle NotFound, missing managed identity, missing contact entry) leave both out-pointers indeterminate. DashPayProfileFFI owns *mut c_char fields released by dashpay_profile_ffi_free — a subsequent free on a slot the caller didn't pre-zero would free garbage pointers across the ABI. Pre-write *out_profile = DashPayProfileFFI::empty(); *out_has_profile = false; immediately after check_ptr! in each function, matching the established_contact_get_alias precedent and the explicit empty-construction already used in the None match arms.

suggestion: FFI get_core_tx_record drops InstantSend rows, defeating reconcile_sent_payments recovery on Swift backend

packages/rs-platform-wallet-ffi/src/persistence.rs (line 2125)

context_kind == 1 still hard-returns Ok(None) with the IS-lock-not-persisted rationale. reconcile_sent_payments treats TransactionContext::InstantSend as final for DashPay display and uses persisted core records to recover Pending Sent entries; this adapter discards them before reconcile sees them. The relaunch-after-IS-lock-but-before-block recovery case therefore remains Pending forever on iOS. The Rust unit tests inject TransactionContext::InstantSend through a custom persister and bypass this fallback. Either reconstruct a synthetic TransactionContext::InstantSend(IsdLock::default()) at this boundary when tx bytes are present, or expose a separate adapter that returns the IS context without requiring the on-disk IS-lock blob.

suggestion: Confirmed-absent contact profiles cannot cross the Swift persistence boundary — stale rows resurrect on cold start

packages/rs-platform-wallet-ffi/src/identity_persistence.rs (line 597)

allocate_contact_profile_rows continues past entries with profile == None ("Skip confirmed-absent entries — the negative cache is not persisted; it rebuilds on the next sweep"). The Rust→Swift FFI projection carries no removal intent for absences; Swift's upsertDashpayContactProfiles is append/refresh-only; and ContactProfileRestoreEntryFFI rehydrates every persisted row as ContactProfileEntry { profile: Some(..) }. When a contact removes their on-chain dashpay.profile, apply_fetched_profile flips the in-memory entry to None, but the absence is dropped at the FFI projection — the stale PersistentDashpayContactProfile row survives on disk. On cold start the cache rehydrates as Some(stale_profile) and the UI re-issues GETs to the previously attacker-controlled avatarUrl, defeating on-chain removal and compounding the unaccepted-sender avatar-fetch tracking channel. Project absences as an explicit delete intent across the boundary (e.g. removed_contact_profile_ids: *const [u8;32] + count), mirroring the ContactIgnoredSenderFFI removal-set shape already established on the same callback.

suggestion: autoAcceptProof FFI copies caller-controlled length before enforcing the 38-102 byte contract

packages/rs-platform-wallet-ffi/src/dashpay.rs (line 241)

platform_wallet_send_contact_request_with_signer accepts auto_accept_proof as a raw pointer plus usize length and immediately builds a Vec with from_raw_parts(..., auto_accept_proof_len).to_vec(). The inner SDK and DashPay docs require proofs in the 38-102 byte range, but that validation happens only after Rust has read and allocated the caller-declared length. With QR-based auto-accept planned for this field, a malformed QR/import path or stale foreign binding can drive an oversized allocation or out-of-bounds read across the FFI boundary before the fixed-size proof contract runs. Reject non-null proofs with len < 38 or len > 102 before forming the slice.

let proof: Option<Vec<u8>> = if auto_accept_proof.is_null() || auto_accept_proof_len == 0 {
        None
    } else {
        if !(38..=102).contains(&auto_accept_proof_len) {
            return PlatformWalletFFIResult::err(
                PlatformWalletFFIResultCode::ErrorInvalidParameter,
                format!(
                    "autoAcceptProof must be 38-102 bytes, got {}",
                    auto_accept_proof_len
                ),
            );
        }
        Some(std::slice::from_raw_parts(auto_accept_proof, auto_accept_proof_len).to_vec())
    };
suggestion: Changeset-end callback errors are logged and discarded — Swift rollback is invisible to Rust round_success

packages/rs-platform-wallet-ffi/src/persistence.rs (line 1493)

Swift's endChangeset catches backgroundContext.save() failures and calls backgroundContext.rollback(), but the C shim changesetEndCallback returns 0 regardless. On the Rust side, lines 1493-1498 only log a non-zero result via eprintln! and never flip round_success based on the callback's return value. The check at 1500 (if !round_success) reacts only to per-kind callback failures recorded earlier in the round, not to the changeset-end outcome. If Swift fails to commit the round and silently rolls back, Rust still considers store() successful, merges the changeset into the long-lived pending accumulator at 1515-1525, and the staged in-memory state diverges from durable SwiftData state. Either propagate the Swift save() outcome through the shim's return code and gate round_success on it, or have the Swift handler surface save failures via a separate error-set callback before the round closes.

suggestion: Contact-profile checked_at_ms not durably persisted on unchanged refresh — negative-cache backoff does not survive restart

packages/rs-platform-wallet/src/wallet/identity/network/profile.rs (line 588)

apply_fetched_profile always refreshes checked_at_ms = now_ms in memory but returns changed only when the stored profile value differs (equality compares only profile content). sync_contact_profiles calls persister.store only when any_changed is true. A re-check confirming the same present or confirmed-absent profile updates the timestamp in memory only — the FFI persistence callback never fires. After cold start, restored rows carry stale checked_at_ms, should_fetch_profile immediately re-issues an In query for every unchanged contact, and the negative-cache backoff effectively does not survive restart. This amplifies the pre-consent avatar tracking channel and creates a cold-start fetch storm; a failed earlier store is also not retried because later sweeps see changed = false. Return a separate needs_persist signal from apply_fetched_profile for timestamp-only refreshes, with coarse coalescing if write-amp matters.

suggestion: Paginated retrieve-all has no per-call total or elapsed budget — algorithmic-DoS surface against the recurring sync loop

packages/rs-sdk/src/platform/dashpay/contact_request_queries.rs (line 41)

fetch_contact_requests_paginated drains every matching document at CONTACT_REQUEST_PAGE_SIZE = 100 per page with no per-call total cap, no elapsed-time budget, and no caller-visible continuation. On cold start, restore-from-seed, or any after_created_at == None path it walks the entire history in one call. unignore_sender resets high_water_received_ms to None, so any UI un-ignore drops the next sweep onto the full retrieve-all. Combined with the recurring DashPaySyncManager tick added in this PR, an attacker who spams contactRequest documents (varying accountReference to defeat per-request dedup) against one of the wallet's identities can make every recurring sweep monopolize the SDK gRPC client and starve every other identity's sync — a cheap algorithmic-DoS against the verified-proof query path. Add a configurable total-document ceiling per sweep with a resumable cursor, or at minimum a logged-warn + early-break above a threshold.

🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

- [BLOCKING] In `packages/rs-platform-wallet/src/wallet/identity/network/profile.rs`:752-766: Contact-profile chunk query omits orderBy required for $ownerId In
  `fetch_contact_profiles_chunk` builds a `$ownerId In [chunk]` query but leaves `order_by_clauses: vec![]` at line 763. `In` is a range operator and DAPI rejects the request with 'missing order by for range error' — every contact-profile chunk fails and is skipped, so the new contact-profile cache/UI never receives remote profiles. The PR's own commit `82c85220ff` documents this as a known bug but has not fixed it. Since this PR ships the contact-profile sync feature that the new SwiftUI Requests tab depends on, the fix is a prerequisite, not a follow-up. Add an `OrderClause { field: "$ownerId", ascending: true }` to `order_by_clauses` (importing `OrderClause` from `dash_sdk::drive::query`).
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/established_contact.rs`:156-169: established_contact_get_note leaves *out_note undefined on early-return paths
  Sibling `established_contact_get_alias` (103-117) writes `*out_alias = std::ptr::null_mut()` immediately after `check_ptr!`, establishing the module's C-ABI contract that out-params are defined-null on early returns. `established_contact_get_note` skips this pre-null write at line 160. Early returns from `unwrap_option_or_return!` (storage NotFound or `contact.note == None`) and `unwrap_result_or_return!` (CString::new failure on interior NUL — reachable from counterparty-controlled note content via `established_contact_set_note`) leave `*out_note` indeterminate across the ABI. A caller that doesn't pre-zero the slot can read a stale pointer or pass it to `platform_wallet_string_free`, risking invalid-free or double-free. One-line fix matches the sibling pattern exactly.
- [SUGGESTION] In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ContactRequestsView.swift`:119-129: Pending incoming-request rows fetch attacker-controlled avatar URLs before user consent
  `sync_contact_profiles` (profile.rs:642) includes `managed.incoming_contact_requests.keys()` in the per-owner fetch plan, populating the contact-profile cache for senders the victim has never accepted. `IncomingRequestRow` feeds `cachedProfile(row.contactIdentityId)?.avatarUrl` into `DashPayAvatarView` → SwiftUI `AsyncImage`, issuing an outbound HTTPS GET to the sender-controlled host on first render — leaking IP, online state, TLS/UA fingerprint, and per-victim URL-token receipt confirmation before any consent action. With `PersistentDashpayContactProfile` SwiftData persistence the channel survives restart; `is_valid_avatar_url` only validates scheme/length. Ignore mutes only after first render. Either suppress remote avatars Swift-side for unaccepted senders (initials-only variant for the Requests tab) or strip `avatarUrl` from the Rust→Swift projection until the sender becomes an established contact.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/dashpay_profile.rs`:90-185: DashPay profile reader trio leaves *out_profile / *out_has_profile undefined on early-return paths
  `managed_identity_get_dashpay_profile`, `platform_wallet_get_dashpay_profile`, and `platform_wallet_get_contact_profile` only assign `*out_profile` / `*out_has_profile` inside the Some/None match arms. After `check_ptr!`, early returns from `unwrap_result_or_return!(read_identifier(..))` and `unwrap_option_or_return!` (handle NotFound, missing managed identity, missing contact entry) leave both out-pointers indeterminate. `DashPayProfileFFI` owns `*mut c_char` fields released by `dashpay_profile_ffi_free` — a subsequent free on a slot the caller didn't pre-zero would free garbage pointers across the ABI. Pre-write `*out_profile = DashPayProfileFFI::empty(); *out_has_profile = false;` immediately after `check_ptr!` in each function, matching the `established_contact_get_alias` precedent and the explicit empty-construction already used in the None match arms.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/persistence.rs`:2125-2133: FFI get_core_tx_record drops InstantSend rows, defeating reconcile_sent_payments recovery on Swift backend
  `context_kind == 1` still hard-returns `Ok(None)` with the IS-lock-not-persisted rationale. `reconcile_sent_payments` treats `TransactionContext::InstantSend` as final for DashPay display and uses persisted core records to recover Pending Sent entries; this adapter discards them before reconcile sees them. The relaunch-after-IS-lock-but-before-block recovery case therefore remains Pending forever on iOS. The Rust unit tests inject `TransactionContext::InstantSend` through a custom persister and bypass this fallback. Either reconstruct a synthetic `TransactionContext::InstantSend(IsdLock::default())` at this boundary when tx bytes are present, or expose a separate adapter that returns the IS context without requiring the on-disk IS-lock blob.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/identity_persistence.rs`:597-641: Confirmed-absent contact profiles cannot cross the Swift persistence boundary — stale rows resurrect on cold start
  `allocate_contact_profile_rows` continues past entries with `profile == None` ("Skip confirmed-absent entries — the negative cache is not persisted; it rebuilds on the next sweep"). The Rust→Swift FFI projection carries no removal intent for absences; Swift's `upsertDashpayContactProfiles` is append/refresh-only; and `ContactProfileRestoreEntryFFI` rehydrates every persisted row as `ContactProfileEntry { profile: Some(..) }`. When a contact removes their on-chain `dashpay.profile`, `apply_fetched_profile` flips the in-memory entry to None, but the absence is dropped at the FFI projection — the stale `PersistentDashpayContactProfile` row survives on disk. On cold start the cache rehydrates as `Some(stale_profile)` and the UI re-issues GETs to the previously attacker-controlled `avatarUrl`, defeating on-chain removal and compounding the unaccepted-sender avatar-fetch tracking channel. Project absences as an explicit delete intent across the boundary (e.g. `removed_contact_profile_ids: *const [u8;32] + count`), mirroring the `ContactIgnoredSenderFFI` removal-set shape already established on the same callback.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/dashpay.rs`:241-244: autoAcceptProof FFI copies caller-controlled length before enforcing the 38-102 byte contract
  `platform_wallet_send_contact_request_with_signer` accepts `auto_accept_proof` as a raw pointer plus `usize` length and immediately builds a Vec with `from_raw_parts(..., auto_accept_proof_len).to_vec()`. The inner SDK and DashPay docs require proofs in the 38-102 byte range, but that validation happens only after Rust has read and allocated the caller-declared length. With QR-based auto-accept planned for this field, a malformed QR/import path or stale foreign binding can drive an oversized allocation or out-of-bounds read across the FFI boundary before the fixed-size proof contract runs. Reject non-null proofs with `len < 38` or `len > 102` before forming the slice.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/persistence.rs`:1493-1504: Changeset-end callback errors are logged and discarded — Swift rollback is invisible to Rust round_success
  Swift's `endChangeset` catches `backgroundContext.save()` failures and calls `backgroundContext.rollback()`, but the C shim `changesetEndCallback` returns 0 regardless. On the Rust side, lines 1493-1498 only log a non-zero result via `eprintln!` and never flip `round_success` based on the callback's return value. The check at 1500 (`if !round_success`) reacts only to per-kind callback failures recorded earlier in the round, not to the changeset-end outcome. If Swift fails to commit the round and silently rolls back, Rust still considers `store()` successful, merges the changeset into the long-lived `pending` accumulator at 1515-1525, and the staged in-memory state diverges from durable SwiftData state. Either propagate the Swift `save()` outcome through the shim's return code and gate `round_success` on it, or have the Swift handler surface save failures via a separate error-set callback before the round closes.
- [SUGGESTION] In `packages/rs-platform-wallet/src/wallet/identity/network/profile.rs`:588-731: Contact-profile checked_at_ms not durably persisted on unchanged refresh — negative-cache backoff does not survive restart
  `apply_fetched_profile` always refreshes `checked_at_ms = now_ms` in memory but returns `changed` only when the stored profile value differs (equality compares only profile content). `sync_contact_profiles` calls `persister.store` only when `any_changed` is true. A re-check confirming the same present or confirmed-absent profile updates the timestamp in memory only — the FFI persistence callback never fires. After cold start, restored rows carry stale `checked_at_ms`, `should_fetch_profile` immediately re-issues an `In` query for every unchanged contact, and the negative-cache backoff effectively does not survive restart. This amplifies the pre-consent avatar tracking channel and creates a cold-start fetch storm; a failed earlier store is also not retried because later sweeps see `changed = false`. Return a separate `needs_persist` signal from `apply_fetched_profile` for timestamp-only refreshes, with coarse coalescing if write-amp matters.
- [SUGGESTION] In `packages/rs-sdk/src/platform/dashpay/contact_request_queries.rs`:41-112: Paginated retrieve-all has no per-call total or elapsed budget — algorithmic-DoS surface against the recurring sync loop
  `fetch_contact_requests_paginated` drains every matching document at `CONTACT_REQUEST_PAGE_SIZE = 100` per page with no per-call total cap, no elapsed-time budget, and no caller-visible continuation. On cold start, restore-from-seed, or any `after_created_at == None` path it walks the entire history in one call. `unignore_sender` resets `high_water_received_ms` to None, so any UI un-ignore drops the next sweep onto the full retrieve-all. Combined with the recurring `DashPaySyncManager` tick added in this PR, an attacker who spams `contactRequest` documents (varying `accountReference` to defeat per-request dedup) against one of the wallet's identities can make every recurring sweep monopolize the SDK gRPC client and starve every other identity's sync — a cheap algorithmic-DoS against the verified-proof query path. Add a configurable total-document ceiling per sweep with a resumable cursor, or at minimum a logged-warn + early-break above a threshold.

Inline dry-run could not load the GitHub PR diff because this PR exceeds GitHub's 20,000-line diff limit, so I posted the same verified findings as a top-level review body.

…ecp256k1/hmac/sha2)

Addresses the review comment "use secp256k1 directly": a consumer that needs
only platform-encryption shouldn't pull in and compile all of dashcore. The
crate now depends on `secp256k1` + `hmac` + `sha2` directly instead of going
through `dashcore::secp256k1` / `dashcore::hashes`.

- `secp256k1` is pinned to "0.30.0" — the exact version dashcore re-exports — so
  the public `SecretKey`/`PublicKey` types still unify with dashcore-typed
  callers (platform-wallet, rs-sdk-ffi build unchanged).
- ECDH uses `secp256k1::ecdh::SharedSecret` (same libsecp256k1 SHA256((y|2)‖x));
  the accountReference HMAC-SHA256 moves from `dashcore::hashes` to `hmac`+`sha2`.
- Behavior is byte-identical: the ECDH and ASK28 known-answer tests pin the exact
  output bytes and pass unchanged.

`cargo tree -p platform-encryption` no longer lists dashcore (only secp256k1,
hmac, sha2, aes, cbc, thiserror). platform-encryption 15/15, platform-wallet
293/9 green; consumers build clean.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Cumulative + delta review at 6d90d73. The latest delta is a pure refactor splitting platform-encryption/src/lib.rs into per-concern modules with re-exports preserving the public API — no behavioral changes, no new defects. All 10 prior findings from the e74e4e2 review re-verified STILL VALID at current HEAD line numbers. Carrying forward the 1 blocking finding (contact-profile chunk query missing orderBy for $ownerId In range operator — feature ships broken) and 9 suggestion-severity findings spanning FFI out-param hygiene, pre-consent avatar fetching, SwiftData rollback divergence, and recurring-sync DoS surface.

🔴 1 blocking | 🟡 9 suggestion(s)

10 additional finding(s)

blocking: Contact-profile chunk query omits orderBy required for $ownerId In

packages/rs-platform-wallet/src/wallet/identity/network/profile.rs (line 752)

Verified at HEAD: fetch_contact_profiles_chunk builds a $ownerId In [chunk] query at lines 756-760 with order_by_clauses: vec![] at line 763. The inline comment at 733-736 ("the profile ownerId index is unique, so the set lookup proves cleanly with an empty order_by") is incorrect — Drive's WhereOperator::In.is_range() returns true (packages/rs-drive/src/query/conditions.rs:128-135), and packages/rs-drive/src/query/mod.rs:2166-2171 returns QuerySyntaxError::MissingOrderByForRange on any range clause without a matching order_by entry, regardless of index uniqueness. Every contact-profile chunk fails on the live network and is swallowed by the per-chunk log-and-continue handler at 718-725, so the contact-profile sync that this PR's SwiftUI Requests/Contacts surface depends on never populates remote profiles. This is a prerequisite for the feature shipping in this PR, not a follow-up. Add OrderClause { field: "$ownerId".to_string(), ascending: true } (imported from dash_sdk::drive::query) to order_by_clauses.

order_by_clauses: vec![dash_sdk::drive::query::OrderClause {
                field: "$ownerId".to_string(),
                ascending: true,
            }],
suggestion: established_contact_get_note leaves *out_note undefined on early-return paths

packages/rs-platform-wallet-ffi/src/established_contact.rs (line 156)

Sibling established_contact_get_alias (103-117) writes *out_alias = std::ptr::null_mut(); immediately after check_ptr! at line 108, establishing the module's C-ABI contract that out-params are defined-null on early returns. established_contact_get_note only calls check_ptr!(out_note) at line 160 and skips the pre-null write. Early returns from unwrap_option_or_return! (storage NotFound, contact.note == None) and unwrap_result_or_return! (CString::new failure on interior NUL — reachable from counterparty-controlled note content via established_contact_set_note) leave *out_note indeterminate across the ABI. A caller that doesn't pre-zero the slot can read a stale pointer or pass it to platform_wallet_string_free, risking invalid-free or double-free.

pub unsafe extern "C" fn established_contact_get_note(
    contact_handle: Handle,
    out_note: *mut *mut std::os::raw::c_char,
) -> PlatformWalletFFIResult {
    check_ptr!(out_note);
    *out_note = std::ptr::null_mut();

    let option =
        ESTABLISHED_CONTACT_STORAGE.with_item(contact_handle, |contact| contact.note.clone());
    let option = unwrap_option_or_return!(option);
    let note = unwrap_option_or_return!(option);
    let c_str = unwrap_result_or_return!(std::ffi::CString::new(note));
    unsafe { *out_note = c_str.into_raw() };
    PlatformWalletFFIResult::ok()
}
suggestion: Pending incoming-request rows fetch attacker-controlled avatar URLs before user consent

packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ContactRequestsView.swift (line 119)

sync_contact_profiles (profile.rs:642) includes managed.incoming_contact_requests.keys() in the per-owner fetch plan, populating the contact-profile cache for senders the victim has never accepted. IncomingRequestRow feeds cachedProfile(row.contactIdentityId)?.avatarUrl into DashPayAvatarView → SwiftUI AsyncImage, issuing an outbound HTTPS GET to the sender-controlled host on first render — leaking IP, online state, TLS/UA fingerprint, and per-victim URL-token receipt confirmation before any consent action. With the PersistentDashpayContactProfile SwiftData persistence introduced by this PR the channel survives restart; Rust-side is_valid_avatar_url only validates scheme/length. Ignore mutes only AFTER first render. Either suppress remote avatars Swift-side for unaccepted senders (initials-only variant for the Requests tab) or strip avatarUrl from the Rust→Swift projection until the sender becomes an established contact.

suggestion: DashPay profile reader trio leaves *out_profile / *out_has_profile undefined on early-return paths

packages/rs-platform-wallet-ffi/src/dashpay_profile.rs (line 90)

managed_identity_get_dashpay_profile (90-112), platform_wallet_get_dashpay_profile (116-145), and platform_wallet_get_contact_profile (153-185) only assign *out_profile / *out_has_profile inside the Some/None match arms. After check_ptr!, early returns from unwrap_result_or_return!(read_identifier(..)) at lines 125, 163-164 and unwrap_option_or_return! (handle NotFound at 100, 132-133, 173) leave both out-pointers indeterminate across the C ABI. DashPayProfileFFI owns *mut c_char fields released by dashpay_profile_ffi_free — a subsequent free on a slot the caller didn't pre-zero would free garbage pointers (invalid-free or double-free). Pre-write *out_profile = DashPayProfileFFI::empty(); *out_has_profile = false; immediately after check_ptr! in each function, matching the established_contact_get_alias precedent and the explicit empty-construction already used in the None match arms.

suggestion: FFI get_core_tx_record drops InstantSend rows, defeating reconcile_sent_payments recovery on Swift backend

packages/rs-platform-wallet-ffi/src/persistence.rs (line 2125)

context_kind == 1 (InstantSend) at line 2127 hard-returns Ok(None) with the IS-lock-not-persisted rationale. reconcile_sent_payments treats TransactionContext::InstantSend as final for DashPay display and uses persisted core records to recover Pending Sent entries; this Swift→Rust FFI adapter discards them before reconcile sees them. The relaunch-after-IS-lock-but-before-block recovery case therefore remains Pending forever on iOS. The Rust unit tests inject TransactionContext::InstantSend through a custom persister and bypass this fallback, so the gap is not exercised in tests. Either reconstruct a synthetic TransactionContext::InstantSend(IsdLock::default()) at this boundary when tx bytes are present, or expose a separate adapter that returns the IS context without requiring the on-disk IS-lock blob.

suggestion: Confirmed-absent contact profiles cannot cross the Swift persistence boundary — stale rows resurrect on cold start

packages/rs-platform-wallet-ffi/src/identity_persistence.rs (line 597)

allocate_contact_profile_rows continues past entries with profile == None at 610-612 ("Skip confirmed-absent entries — the negative cache is not persisted; it rebuilds on the next sweep"). The Rust→Swift FFI projection carries no removal intent for absences; Swift's upsertDashpayContactProfiles is append/refresh-only; and ContactProfileRestoreEntryFFI rehydrates every persisted row as ContactProfileEntry { profile: Some(..) }. When a contact removes their on-chain dashpay.profile, apply_fetched_profile flips the in-memory entry to None, but the absence is dropped at the FFI projection — the stale PersistentDashpayContactProfile row survives on disk. On cold start the cache rehydrates as Some(stale_profile) and the UI re-issues GETs to the previously attacker-controlled avatarUrl, defeating on-chain removal and compounding the unaccepted-sender avatar-fetch tracking channel. Project absences as an explicit delete intent across the boundary (e.g. removed_contact_profile_ids: *const [u8;32] + count), mirroring the ContactIgnoredSenderFFI removal-set shape this PR already establishes on the same callback.

suggestion: autoAcceptProof FFI copies caller-controlled length before enforcing the 38-102 byte contract

packages/rs-platform-wallet-ffi/src/dashpay.rs (line 241)

platform_wallet_send_contact_request_with_signer accepts auto_accept_proof as a raw pointer plus usize length and immediately builds a Vec with std::slice::from_raw_parts(auto_accept_proof, auto_accept_proof_len).to_vec() at line 244. The inner SDK and DashPay docs require proofs in the 38-102 byte range, but that validation runs only after Rust has read and allocated the caller-declared length. With QR-based auto-accept planned for this field, a malformed QR/import path or stale foreign binding can drive an oversized allocation or out-of-bounds read across the C ABI boundary before the fixed-size proof contract runs. Reject non-null proofs with len < 38 or len > 102 at the boundary before forming the slice.

let proof: Option<Vec<u8>> = if auto_accept_proof.is_null() || auto_accept_proof_len == 0 {
        None
    } else {
        if !(38..=102).contains(&auto_accept_proof_len) {
            return PlatformWalletFFIResult::err(
                PlatformWalletFFIResultCode::ErrorInvalidParameter,
                format!(
                    "autoAcceptProof must be 38-102 bytes, got {}",
                    auto_accept_proof_len
                ),
            );
        }
        Some(std::slice::from_raw_parts(auto_accept_proof, auto_accept_proof_len).to_vec())
    };
suggestion: Changeset-end callback errors are logged and discarded — Swift rollback is invisible to Rust round_success

packages/rs-platform-wallet-ffi/src/persistence.rs (line 1493)

Swift's endChangeset catches backgroundContext.save() failures and calls backgroundContext.rollback(), but the C shim changesetEndCallback returns 0 regardless. On the Rust side, lines 1493-1498 only eprintln! a non-zero result and never flip round_success based on the callback's return value. The check at 1500 reacts only to per-kind callback failures recorded earlier in the round, not to the changeset-end outcome. If Swift fails to commit the round and silently rolls back, Rust still considers store() successful, merges the changeset into the long-lived pending accumulator at 1515-1525, and the staged in-memory state diverges from durable SwiftData state. Either propagate the Swift save() outcome through the shim's return code and gate round_success on it, or have the Swift handler surface save failures via a separate error-set callback before the round closes.

suggestion: Contact-profile checked_at_ms not durably persisted on unchanged refresh — negative-cache backoff does not survive restart

packages/rs-platform-wallet/src/wallet/identity/network/profile.rs (line 588)

apply_fetched_profile always refreshes checked_at_ms = now_ms in memory but returns changed only when the stored profile value differs (equality compares only profile content). sync_contact_profiles calls persister.store only when any_changed is true (718-725). A re-check confirming the same present or confirmed-absent profile updates the timestamp in memory only — the FFI persistence callback never fires across the Rust→Swift boundary. After cold start, restored rows carry stale checked_at_ms, should_fetch_profile immediately re-issues an In query for every unchanged contact, and the negative-cache backoff effectively does not survive restart. This amplifies the pre-consent avatar tracking channel and creates a cold-start fetch storm; a failed earlier store is also not retried because later sweeps see changed = false. Return a separate needs_persist signal from apply_fetched_profile for timestamp-only refreshes, with coarse coalescing if write-amp matters.

suggestion: Paginated retrieve-all has no per-call total or elapsed budget — algorithmic-DoS surface against the recurring sync loop

packages/rs-sdk/src/platform/dashpay/contact_request_queries.rs (line 41)

fetch_contact_requests_paginated drains every matching document at CONTACT_REQUEST_PAGE_SIZE = 100 per page with no per-call total cap, no elapsed-time budget, and no caller-visible continuation. On cold start, restore-from-seed, or any after_created_at == None path it walks the entire history in one call. unignore_sender resets high_water_received_ms to None, so any UI un-ignore drops the next sweep onto the full retrieve-all. Combined with the recurring DashPaySyncManager tick added in this PR, an attacker who spams contactRequest documents (varying accountReference to defeat per-request dedup) against one of the wallet's identities can make every recurring sweep monopolize the SDK gRPC client and starve every other identity's sync — a cheap algorithmic-DoS against the verified-proof query path. Add a configurable total-document ceiling per sweep with a resumable cursor, or at minimum a logged-warn + early-break above a threshold.

🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

- [BLOCKING] In `packages/rs-platform-wallet/src/wallet/identity/network/profile.rs`:752-766: Contact-profile chunk query omits orderBy required for $ownerId In
  Verified at HEAD: `fetch_contact_profiles_chunk` builds a `$ownerId In [chunk]` query at lines 756-760 with `order_by_clauses: vec![]` at line 763. The inline comment at 733-736 ("the `profile` `ownerId` index is unique, so the set lookup proves cleanly with an empty `order_by`") is incorrect — Drive's `WhereOperator::In.is_range()` returns true (packages/rs-drive/src/query/conditions.rs:128-135), and packages/rs-drive/src/query/mod.rs:2166-2171 returns `QuerySyntaxError::MissingOrderByForRange` on any range clause without a matching `order_by` entry, regardless of index uniqueness. Every contact-profile chunk fails on the live network and is swallowed by the per-chunk log-and-continue handler at 718-725, so the contact-profile sync that this PR's SwiftUI Requests/Contacts surface depends on never populates remote profiles. This is a prerequisite for the feature shipping in this PR, not a follow-up. Add `OrderClause { field: "$ownerId".to_string(), ascending: true }` (imported from `dash_sdk::drive::query`) to `order_by_clauses`.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/established_contact.rs`:156-169: established_contact_get_note leaves *out_note undefined on early-return paths
  Sibling `established_contact_get_alias` (103-117) writes `*out_alias = std::ptr::null_mut();` immediately after `check_ptr!` at line 108, establishing the module's C-ABI contract that out-params are defined-null on early returns. `established_contact_get_note` only calls `check_ptr!(out_note)` at line 160 and skips the pre-null write. Early returns from `unwrap_option_or_return!` (storage NotFound, `contact.note == None`) and `unwrap_result_or_return!` (CString::new failure on interior NUL — reachable from counterparty-controlled note content via `established_contact_set_note`) leave `*out_note` indeterminate across the ABI. A caller that doesn't pre-zero the slot can read a stale pointer or pass it to `platform_wallet_string_free`, risking invalid-free or double-free.
- [SUGGESTION] In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ContactRequestsView.swift`:119-129: Pending incoming-request rows fetch attacker-controlled avatar URLs before user consent
  `sync_contact_profiles` (profile.rs:642) includes `managed.incoming_contact_requests.keys()` in the per-owner fetch plan, populating the contact-profile cache for senders the victim has never accepted. `IncomingRequestRow` feeds `cachedProfile(row.contactIdentityId)?.avatarUrl` into `DashPayAvatarView` → SwiftUI `AsyncImage`, issuing an outbound HTTPS GET to the sender-controlled host on first render — leaking IP, online state, TLS/UA fingerprint, and per-victim URL-token receipt confirmation before any consent action. With the `PersistentDashpayContactProfile` SwiftData persistence introduced by this PR the channel survives restart; Rust-side `is_valid_avatar_url` only validates scheme/length. Ignore mutes only AFTER first render. Either suppress remote avatars Swift-side for unaccepted senders (initials-only variant for the Requests tab) or strip `avatarUrl` from the Rust→Swift projection until the sender becomes an established contact.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/dashpay_profile.rs`:90-185: DashPay profile reader trio leaves *out_profile / *out_has_profile undefined on early-return paths
  `managed_identity_get_dashpay_profile` (90-112), `platform_wallet_get_dashpay_profile` (116-145), and `platform_wallet_get_contact_profile` (153-185) only assign `*out_profile` / `*out_has_profile` inside the Some/None match arms. After `check_ptr!`, early returns from `unwrap_result_or_return!(read_identifier(..))` at lines 125, 163-164 and `unwrap_option_or_return!` (handle NotFound at 100, 132-133, 173) leave both out-pointers indeterminate across the C ABI. `DashPayProfileFFI` owns `*mut c_char` fields released by `dashpay_profile_ffi_free` — a subsequent free on a slot the caller didn't pre-zero would free garbage pointers (invalid-free or double-free). Pre-write `*out_profile = DashPayProfileFFI::empty(); *out_has_profile = false;` immediately after `check_ptr!` in each function, matching the `established_contact_get_alias` precedent and the explicit empty-construction already used in the None match arms.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/persistence.rs`:2125-2133: FFI get_core_tx_record drops InstantSend rows, defeating reconcile_sent_payments recovery on Swift backend
  `context_kind == 1` (InstantSend) at line 2127 hard-returns `Ok(None)` with the IS-lock-not-persisted rationale. `reconcile_sent_payments` treats `TransactionContext::InstantSend` as final for DashPay display and uses persisted core records to recover Pending Sent entries; this Swift→Rust FFI adapter discards them before reconcile sees them. The relaunch-after-IS-lock-but-before-block recovery case therefore remains Pending forever on iOS. The Rust unit tests inject `TransactionContext::InstantSend` through a custom persister and bypass this fallback, so the gap is not exercised in tests. Either reconstruct a synthetic `TransactionContext::InstantSend(IsdLock::default())` at this boundary when tx bytes are present, or expose a separate adapter that returns the IS context without requiring the on-disk IS-lock blob.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/identity_persistence.rs`:597-641: Confirmed-absent contact profiles cannot cross the Swift persistence boundary — stale rows resurrect on cold start
  `allocate_contact_profile_rows` `continue`s past entries with `profile == None` at 610-612 ("Skip confirmed-absent entries — the negative cache is not persisted; it rebuilds on the next sweep"). The Rust→Swift FFI projection carries no removal intent for absences; Swift's `upsertDashpayContactProfiles` is append/refresh-only; and `ContactProfileRestoreEntryFFI` rehydrates every persisted row as `ContactProfileEntry { profile: Some(..) }`. When a contact removes their on-chain `dashpay.profile`, `apply_fetched_profile` flips the in-memory entry to None, but the absence is dropped at the FFI projection — the stale `PersistentDashpayContactProfile` row survives on disk. On cold start the cache rehydrates as `Some(stale_profile)` and the UI re-issues GETs to the previously attacker-controlled `avatarUrl`, defeating on-chain removal and compounding the unaccepted-sender avatar-fetch tracking channel. Project absences as an explicit delete intent across the boundary (e.g. `removed_contact_profile_ids: *const [u8;32] + count`), mirroring the `ContactIgnoredSenderFFI` removal-set shape this PR already establishes on the same callback.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/dashpay.rs`:241-245: autoAcceptProof FFI copies caller-controlled length before enforcing the 38-102 byte contract
  `platform_wallet_send_contact_request_with_signer` accepts `auto_accept_proof` as a raw pointer plus `usize` length and immediately builds a Vec with `std::slice::from_raw_parts(auto_accept_proof, auto_accept_proof_len).to_vec()` at line 244. The inner SDK and DashPay docs require proofs in the 38-102 byte range, but that validation runs only after Rust has read and allocated the caller-declared length. With QR-based auto-accept planned for this field, a malformed QR/import path or stale foreign binding can drive an oversized allocation or out-of-bounds read across the C ABI boundary before the fixed-size proof contract runs. Reject non-null proofs with `len < 38` or `len > 102` at the boundary before forming the slice.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/persistence.rs`:1493-1504: Changeset-end callback errors are logged and discarded — Swift rollback is invisible to Rust round_success
  Swift's `endChangeset` catches `backgroundContext.save()` failures and calls `backgroundContext.rollback()`, but the C shim `changesetEndCallback` returns 0 regardless. On the Rust side, lines 1493-1498 only `eprintln!` a non-zero result and never flip `round_success` based on the callback's return value. The check at 1500 reacts only to per-kind callback failures recorded earlier in the round, not to the changeset-end outcome. If Swift fails to commit the round and silently rolls back, Rust still considers `store()` successful, merges the changeset into the long-lived `pending` accumulator at 1515-1525, and the staged in-memory state diverges from durable SwiftData state. Either propagate the Swift `save()` outcome through the shim's return code and gate `round_success` on it, or have the Swift handler surface save failures via a separate error-set callback before the round closes.
- [SUGGESTION] In `packages/rs-platform-wallet/src/wallet/identity/network/profile.rs`:588-731: Contact-profile checked_at_ms not durably persisted on unchanged refresh — negative-cache backoff does not survive restart
  `apply_fetched_profile` always refreshes `checked_at_ms = now_ms` in memory but returns `changed` only when the stored profile value differs (equality compares only profile content). `sync_contact_profiles` calls `persister.store` only when `any_changed` is true (718-725). A re-check confirming the same present or confirmed-absent profile updates the timestamp in memory only — the FFI persistence callback never fires across the Rust→Swift boundary. After cold start, restored rows carry stale `checked_at_ms`, `should_fetch_profile` immediately re-issues an `In` query for every unchanged contact, and the negative-cache backoff effectively does not survive restart. This amplifies the pre-consent avatar tracking channel and creates a cold-start fetch storm; a failed earlier store is also not retried because later sweeps see `changed = false`. Return a separate `needs_persist` signal from `apply_fetched_profile` for timestamp-only refreshes, with coarse coalescing if write-amp matters.
- [SUGGESTION] In `packages/rs-sdk/src/platform/dashpay/contact_request_queries.rs`:41-112: Paginated retrieve-all has no per-call total or elapsed budget — algorithmic-DoS surface against the recurring sync loop
  `fetch_contact_requests_paginated` drains every matching document at `CONTACT_REQUEST_PAGE_SIZE = 100` per page with no per-call total cap, no elapsed-time budget, and no caller-visible continuation. On cold start, restore-from-seed, or any `after_created_at == None` path it walks the entire history in one call. `unignore_sender` resets `high_water_received_ms` to None, so any UI un-ignore drops the next sweep onto the full retrieve-all. Combined with the recurring `DashPaySyncManager` tick added in this PR, an attacker who spams `contactRequest` documents (varying `accountReference` to defeat per-request dedup) against one of the wallet's identities can make every recurring sweep monopolize the SDK gRPC client and starve every other identity's sync — a cheap algorithmic-DoS against the verified-proof query path. Add a configurable total-document ceiling per sweep with a resumable cursor, or at minimum a logged-warn + early-break above a threshold.

Inline dry-run could not load the GitHub PR diff because this PR exceeds GitHub's 20,000-line diff limit, so I posted the same verified findings as a top-level review body.

…ync manager

Addresses two review comments on the DashPay sync layer:

- "why you talking about tokens here?" — tightened the DashPaySyncManager module
  doc so the token reference is a one-line "why not IdentitySyncManager" rationale
  (it skips token-less identities, so a DashPay-only identity wouldn't sync),
  not a registry-internals digression.

- "won't it be better to move to the sync manager we added?" — moved the
  per-wallet DashPay orchestration into the coordinator as
  `DashPaySyncManager::sync_wallet_dashpay`, and deleted the
  `IdentityWallet::dashpay_sync` aggregator (its only caller was the sweep). The
  manager now owns the sync flow (sequencing + log-and-continue); the six steps
  (contact requests → own profiles → contact profiles → contactInfo →
  incoming/sent reconciles) stay as `IdentityWallet` domain operations — they
  mutate identity/contact state and each has standalone on-demand FFI callers
  (`sync_contact_requests`, `sync_profiles`), so they belong on the entity, not
  the scheduler. Behaviour (ordering, error-surfacing, log-and-continue) is
  preserved verbatim.

platform-wallet 293/9, glue 115/26/5 green.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Verified against the checked-out df5f03e worktree. All 10 prior Codex findings still reproduce and are in scope for this DashPay completion PR; the contact-profile $ownerId In query remains blocking because Drive requires an orderBy for range operators. CodeRabbit supplied no inline findings, so no reactions are needed.

🔴 1 blocking | 🟡 9 suggestion(s)

10 additional finding(s)

blocking: Contact-profile chunk query omits required orderBy

packages/rs-platform-wallet/src/wallet/identity/network/profile.rs (line 752)

fetch_contact_profiles_chunk builds a profile query with $ownerId In [...] but leaves order_by_clauses empty. In Drive, In is a range operator, and range clauses require a matching orderBy; the unique ownerId index does not bypass that validation. DAPI rejects this query with the missing-order-by-for-range error, and the caller logs the chunk failure and continues, so the new DashPay contact-profile sync cannot populate remote profiles for the UI.

order_by_clauses: vec![dash_sdk::drive::query::OrderClause {
                field: "$ownerId".to_string(),
                ascending: true,
            }],
suggestion: established_contact_get_note leaves out_note undefined on early returns

packages/rs-platform-wallet-ffi/src/established_contact.rs (line 156)

The C ABI checks that out_note is non-null, then performs fallible storage lookup, option unwrap, and CString construction before writing the output pointer. If any of those paths returns NotFound or an error, callers that did not pre-zero the slot can observe or free a stale pointer. The sibling alias getter already pre-nulls its out-param; this getter should do the same.

pub unsafe extern "C" fn established_contact_get_note(
    contact_handle: Handle,
    out_note: *mut *mut std::os::raw::c_char,
) -> PlatformWalletFFIResult {
    check_ptr!(out_note);
    *out_note = std::ptr::null_mut();

    let option =
        ESTABLISHED_CONTACT_STORAGE.with_item(contact_handle, |contact| contact.note.clone());
    let option = unwrap_option_or_return!(option);
    let note = unwrap_option_or_return!(option);
    let c_str = unwrap_result_or_return!(std::ffi::CString::new(note));
    unsafe { *out_note = c_str.into_raw() };
    PlatformWalletFFIResult::ok()
}
suggestion: Incoming requests load sender-controlled avatars before consent

packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ContactRequestsView.swift (line 119)

The PR's profile sync includes pending incoming request senders, then the Requests screen passes cachedProfile(row.contactIdentityId)?.avatarUrl into IncomingRequestRow. That row ultimately uses SwiftUI AsyncImage, so an unsolicited sender can publish a per-victim HTTPS avatar URL, send a contact request, and learn when the victim opens the Requests tab before any accept or ignore action. Suppress remote avatars for unaccepted incoming senders, or avoid projecting avatarUrl until the contact is established.

suggestion: DashPay profile getters leave owned out-params undefined

packages/rs-platform-wallet-ffi/src/dashpay_profile.rs (line 90)

managed_identity_get_dashpay_profile, platform_wallet_get_dashpay_profile, and platform_wallet_get_contact_profile validate the output pointers but only assign out_profile and out_has_profile after fallible identifier parsing and handle/storage lookups. Current Swift wrappers often initialize locals first, but the exported C ABI itself should define outputs on every return path because DashPayProfileFFI contains owned string pointers later freed by dashpay_profile_ffi_free. Initialize out_profile to DashPayProfileFFI::empty() and out_has_profile to false immediately after pointer validation in each function.

suggestion: InstantSend transaction records are dropped on restore

packages/rs-platform-wallet-ffi/src/persistence.rs (line 2125)

Swift persistence returns context kind 1 together with transaction bytes for Mempool/InstantSend rows, but FFIPersister maps context_kind == 1 to Ok(None) before decoding the transaction. The DashPay sent-payment reconciliation path treats InstantSend as display-ready state, so a payment persisted after InstantSend but before a block context can be hidden from Rust after relaunch and remain pending. Return a recoverable context when transaction bytes are present, or expose a separate persisted state that reconciliation can consume without requiring the IS-lock blob.

suggestion: Confirmed-absent contact profiles cannot delete persisted rows

packages/rs-platform-wallet-ffi/src/identity_persistence.rs (line 597)

Rust represents a confirmed profile absence as ContactProfileEntry { profile: None }, but allocate_contact_profile_rows skips those entries entirely. The Swift side only upserts present profiles, and restore rehydrates every persisted contact-profile row as present. If a contact removes their on-chain DashPay profile, the stale persisted profile, including any old avatar URL, survives on disk and reappears after cold start. Project explicit delete/removal intents across the FFI boundary.

suggestion: autoAcceptProof is copied before validating the proof size

packages/rs-platform-wallet-ffi/src/dashpay.rs (line 241)

platform_wallet_send_contact_request_with_signer accepts a raw pointer plus length and immediately reads auto_accept_proof_len bytes into a Vec. The DashPay proof contract is 38-102 bytes, but that validation happens after Rust has already trusted the foreign length. A malformed binding or import path can trigger oversized allocation or an out-of-bounds FFI read before the proof is rejected.

let proof: Option<Vec<u8>> = if auto_accept_proof.is_null() || auto_accept_proof_len == 0 {
        None
    } else {
        if !(38..=102).contains(&auto_accept_proof_len) {
            return PlatformWalletFFIResult::err(
                PlatformWalletFFIResultCode::ErrorInvalidParameter,
                format!(
                    "autoAcceptProof must be 38-102 bytes, got {}",
                    auto_accept_proof_len
                ),
            );
        }
        Some(std::slice::from_raw_parts(auto_accept_proof, auto_accept_proof_len).to_vec())
    };
suggestion: changeset-end callback failure is not propagated

packages/rs-platform-wallet-ffi/src/persistence.rs (line 1493)

The per-kind persistence callbacks set round_success = false on failure, but the final on_changeset_end_fn result is only logged. If the Swift side fails to commit or rolls back during changeset end, Rust still sees round_success == true, returns success, and merges the changeset into the long-lived pending accumulator. Treat a non-zero changeset-end result as a failed round before the success check.

suggestion: Profile refresh timestamps are not persisted when content is unchanged

packages/rs-platform-wallet/src/wallet/identity/network/profile.rs (line 588)

apply_fetched_profile refreshes checked_at_ms in memory on every successful fetch, but returns true only when profile content changes. sync_contact_profiles persists only when any_changed is true, so unchanged present profiles and confirmed-absent negative-cache refreshes never update durable SwiftData state. After restart, stale checkedAtMs values are restored and the client can refetch immediately, defeating the refresh window and negative-cache backoff across cold starts.

suggestion: Contact-request pagination has no per-sweep budget

packages/rs-sdk/src/platform/dashpay/contact_request_queries.rs (line 41)

fetch_contact_requests_paginated drains every matching contactRequest page until exhaustion, accumulating all documents in memory and returning no continuation to the caller. A spammed identity can force cold-start, restore, or recurring DashPay sync to spend an unbounded amount of time and proof verification work in one sweep. Add a per-sweep document or elapsed-time cap with a resumable cursor, or at minimum stop and warn above a defensive threshold.

🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

- [BLOCKING] In `packages/rs-platform-wallet/src/wallet/identity/network/profile.rs`:752-766: Contact-profile chunk query omits required orderBy
  fetch_contact_profiles_chunk builds a profile query with `$ownerId In [...]` but leaves `order_by_clauses` empty. In Drive, `In` is a range operator, and range clauses require a matching orderBy; the unique ownerId index does not bypass that validation. DAPI rejects this query with the missing-order-by-for-range error, and the caller logs the chunk failure and continues, so the new DashPay contact-profile sync cannot populate remote profiles for the UI.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/established_contact.rs`:156-168: established_contact_get_note leaves out_note undefined on early returns
  The C ABI checks that `out_note` is non-null, then performs fallible storage lookup, option unwrap, and CString construction before writing the output pointer. If any of those paths returns NotFound or an error, callers that did not pre-zero the slot can observe or free a stale pointer. The sibling alias getter already pre-nulls its out-param; this getter should do the same.
- [SUGGESTION] In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ContactRequestsView.swift`:119-128: Incoming requests load sender-controlled avatars before consent
  The PR's profile sync includes pending incoming request senders, then the Requests screen passes `cachedProfile(row.contactIdentityId)?.avatarUrl` into `IncomingRequestRow`. That row ultimately uses SwiftUI AsyncImage, so an unsolicited sender can publish a per-victim HTTPS avatar URL, send a contact request, and learn when the victim opens the Requests tab before any accept or ignore action. Suppress remote avatars for unaccepted incoming senders, or avoid projecting avatarUrl until the contact is established.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/dashpay_profile.rs`:90-185: DashPay profile getters leave owned out-params undefined
  managed_identity_get_dashpay_profile, platform_wallet_get_dashpay_profile, and platform_wallet_get_contact_profile validate the output pointers but only assign `out_profile` and `out_has_profile` after fallible identifier parsing and handle/storage lookups. Current Swift wrappers often initialize locals first, but the exported C ABI itself should define outputs on every return path because `DashPayProfileFFI` contains owned string pointers later freed by `dashpay_profile_ffi_free`. Initialize `out_profile` to `DashPayProfileFFI::empty()` and `out_has_profile` to false immediately after pointer validation in each function.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/persistence.rs`:2125-2133: InstantSend transaction records are dropped on restore
  Swift persistence returns context kind 1 together with transaction bytes for Mempool/InstantSend rows, but FFIPersister maps `context_kind == 1` to `Ok(None)` before decoding the transaction. The DashPay sent-payment reconciliation path treats InstantSend as display-ready state, so a payment persisted after InstantSend but before a block context can be hidden from Rust after relaunch and remain pending. Return a recoverable context when transaction bytes are present, or expose a separate persisted state that reconciliation can consume without requiring the IS-lock blob.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/identity_persistence.rs`:597-641: Confirmed-absent contact profiles cannot delete persisted rows
  Rust represents a confirmed profile absence as `ContactProfileEntry { profile: None }`, but allocate_contact_profile_rows skips those entries entirely. The Swift side only upserts present profiles, and restore rehydrates every persisted contact-profile row as present. If a contact removes their on-chain DashPay profile, the stale persisted profile, including any old avatar URL, survives on disk and reappears after cold start. Project explicit delete/removal intents across the FFI boundary.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/dashpay.rs`:241-245: autoAcceptProof is copied before validating the proof size
  platform_wallet_send_contact_request_with_signer accepts a raw pointer plus length and immediately reads `auto_accept_proof_len` bytes into a Vec. The DashPay proof contract is 38-102 bytes, but that validation happens after Rust has already trusted the foreign length. A malformed binding or import path can trigger oversized allocation or an out-of-bounds FFI read before the proof is rejected.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/persistence.rs`:1493-1504: changeset-end callback failure is not propagated
  The per-kind persistence callbacks set `round_success = false` on failure, but the final `on_changeset_end_fn` result is only logged. If the Swift side fails to commit or rolls back during changeset end, Rust still sees `round_success == true`, returns success, and merges the changeset into the long-lived pending accumulator. Treat a non-zero changeset-end result as a failed round before the success check.
- [SUGGESTION] In `packages/rs-platform-wallet/src/wallet/identity/network/profile.rs`:588-731: Profile refresh timestamps are not persisted when content is unchanged
  apply_fetched_profile refreshes `checked_at_ms` in memory on every successful fetch, but returns true only when profile content changes. sync_contact_profiles persists only when `any_changed` is true, so unchanged present profiles and confirmed-absent negative-cache refreshes never update durable SwiftData state. After restart, stale checkedAtMs values are restored and the client can refetch immediately, defeating the refresh window and negative-cache backoff across cold starts.
- [SUGGESTION] In `packages/rs-sdk/src/platform/dashpay/contact_request_queries.rs`:41-112: Contact-request pagination has no per-sweep budget
  fetch_contact_requests_paginated drains every matching contactRequest page until exhaustion, accumulating all documents in memory and returning no continuation to the caller. A spammed identity can force cold-start, restore, or recurring DashPay sync to spend an unbounded amount of time and proof verification work in one sweep. Add a per-sweep document or elapsed-time cap with a resumable cursor, or at minimum stop and warn above a defensive threshold.

Inline dry-run could not load the GitHub PR diff because this PR exceeds GitHub's 20,000-line diff limit, so I posted the same verified findings as a top-level review body.

The `$ownerId In [chunk]` profile fetch used `order_by_clauses: vec![]`.
`In` is a range operator, and DAPI requires a matching `orderBy` on every
range field, so the query was rejected on-device with "missing order by for
range error: query must have an orderBy field for each range element" —
contact profiles never loaded.

Extract the query into `contact_profiles_chunk_query` and add an
`OrderClause` on `$ownerId`. The `$ownerId` index is unique (≤1 profile per
owner), so ordering does not change the result set; the orderBy only
satisfies the range-orderBy rule.

Regression test asserts every range where-clause carries a matching orderBy,
with a guard that a range clause exists so it can't pass vacuously:
✖ before fix (empty order_by), ✔ after.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Incremental review at a425f70: the latest delta is an internal refactor that moves DashPay sync orchestration into DashPaySyncManager and introduces no new defects, but all 10 prior findings from df5f03e still reproduce verbatim, including the blocking contact-profile chunk query bug that prevents the new profile sync from returning any results on live DAPI. Carrying every STILL VALID prior finding forward; only codex agents produced output this run (claude lanes were unparseable).

🔴 1 blocking | 🟡 9 suggestion(s)

10 additional finding(s)

blocking: Contact-profile chunk query omits required orderBy for In clause

packages/rs-platform-wallet/src/wallet/identity/network/profile.rs (line 752)

fetch_contact_profiles_chunk builds $ownerId In [...] with order_by_clauses: vec![]. Drive treats In as a range operator and requires a matching orderBy — the unique ownerId index does not bypass that validation, so DAPI rejects the query with the missing-order-by-for-range error. The caller logs the chunk failure and continues, so the new DashPay contact-profile sync silently fails for every owner and the UI never gets remote profiles. Add an ascending $ownerId order clause.

        order_by_clauses: vec![dash_sdk::drive::query::OrderClause {
                field: "$ownerId".to_string(),
                ascending: true,
            }],
suggestion: established_contact_get_note leaves out_note undefined on early returns

packages/rs-platform-wallet-ffi/src/established_contact.rs (line 156)

The C ABI validates that out_note is non-null, then performs fallible storage lookup, option unwrap, and CString construction before writing the output pointer. NotFound, no-note, or interior-NUL paths return without defining *out_note, so a foreign caller that did not pre-zero the slot can read or free a stale pointer through platform_wallet_string_free. The sibling alias getter already pre-nulls its out-param; this getter should match.

pub unsafe extern "C" fn established_contact_get_note(
    contact_handle: Handle,
    out_note: *mut *mut std::os::raw::c_char,
) -> PlatformWalletFFIResult {
    check_ptr!(out_note);
    *out_note = std::ptr::null_mut();

    let option =
        ESTABLISHED_CONTACT_STORAGE.with_item(contact_handle, |contact| contact.note.clone());
    let option = unwrap_option_or_return!(option);
    let note = unwrap_option_or_return!(option);
    let c_str = unwrap_result_or_return!(std::ffi::CString::new(note));
    unsafe { *out_note = c_str.into_raw() };
    PlatformWalletFFIResult::ok()
}
suggestion: Incoming requests load sender-controlled avatars before consent

packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ContactRequestsView.swift (line 119)

sync_contact_profiles fetches profiles for managed.incoming_contact_requests.keys() (profile.rs:640-642), and ContactRequestsView passes cachedProfile(row.contactIdentityId)?.avatarUrl into IncomingRequestRow / DashPayAvatarView, which loads via SwiftUI AsyncImage. An unsolicited sender can publish a per-victim HTTPS avatar URL, send a contact request, and learn when the victim opens the Requests tab from the outbound image fetch — before any accept/ignore action. Suppress remote avatar URLs for pending incoming senders, or drop avatarUrl from the Rust→Swift projection until the contact is established.

suggestion: DashPay profile getters leave owned out-params undefined on early returns

packages/rs-platform-wallet-ffi/src/dashpay_profile.rs (line 90)

managed_identity_get_dashpay_profile, platform_wallet_get_dashpay_profile, and platform_wallet_get_contact_profile validate out_profile/out_has_profile but only assign them after fallible identifier parsing and handle/storage lookups. DashPayProfileFFI contains owned C string pointers later released by dashpay_profile_ffi_free, so a foreign caller that reuses an uninitialized slot can free garbage or stale pointers when a NotFound path fires. Initialize *out_profile = DashPayProfileFFI::empty() and *out_has_profile = false immediately after pointer validation in all three functions.

suggestion: InstantSend transaction records are dropped on restore

packages/rs-platform-wallet-ffi/src/persistence.rs (line 2125)

Swift PlatformWalletPersistenceHandler.getCoreTxRecordCallback returns context kind 1 along with transaction bytes for InstantSend rows, but FFIPersister maps context_kind == 1 to Ok(None) before decoding them. The DashPay sent-payment reconciliation path treats InstantSend as a display-ready state, so a payment persisted after InstantSend but before any block context disappears across the boundary on relaunch and remains stuck pending. Return a recoverable context when the transaction bytes are present, or expose a separate persisted state reconciliation can consume without the IS-lock blob.

suggestion: Confirmed-absent contact profiles cannot delete persisted rows

packages/rs-platform-wallet-ffi/src/identity_persistence.rs (line 597)

ContactProfileEntry { profile: None } represents a confirmed absence, but allocate_contact_profile_rows skips those entries entirely (line 610-612). Swift only ever receives present profiles and restore rehydrates every persisted row as profile: Some(...), so if a contact removes their on-chain DashPay profile the stale avatarUrl (potentially attacker-controlled) survives in SwiftData and reappears after cold start, keeping the pre-consent tracking channel alive after the public profile is gone. Project explicit delete/removal intents across the FFI boundary for absent profiles.

suggestion: autoAcceptProof is copied before validating the proof size

packages/rs-platform-wallet-ffi/src/dashpay.rs (line 241)

platform_wallet_send_contact_request_with_signer reads auto_accept_proof_len bytes into a Vec via slice::from_raw_parts before the DashPay proof-size contract (38–102 bytes) is enforced. A malformed binding or import path can trigger oversized allocation or an out-of-bounds FFI read before the proof is rejected. Reject lengths outside 38..=102 before forming the slice.

    let proof: Option<Vec<u8>> = if auto_accept_proof.is_null() || auto_accept_proof_len == 0 {
        None
    } else {
        if !(38..=102).contains(&auto_accept_proof_len) {
            return PlatformWalletFFIResult::err(
                PlatformWalletFFIResultCode::ErrorInvalidParameter,
                format!(
                    "autoAcceptProof must be 38-102 bytes, got {}",
                    auto_accept_proof_len
                ),
            );
        }
        Some(std::slice::from_raw_parts(auto_accept_proof, auto_accept_proof_len).to_vec())
    };
suggestion: changeset-end callback failure is not propagated into round_success

packages/rs-platform-wallet-ffi/src/persistence.rs (line 1493)

Per-kind callbacks set round_success = false on failure, but the final on_changeset_end_fn result is only printed to stderr (line 1495-1497). If the Swift commit fails or rolls back during changeset-end, Rust still sees round_success == true, returns success, and merges the changeset into its long-lived pending accumulator — durable SwiftData state and Rust in-memory pending state silently diverge. Treat a non-zero changeset-end result as a failed round before the !round_success check (and have Swift propagate save failure as a non-zero return).

suggestion: Profile refresh timestamps are not persisted when content is unchanged

packages/rs-platform-wallet/src/wallet/identity/network/profile.rs (line 588)

apply_fetched_profile refreshes checked_at_ms in memory on every successful fetch but returns true only when profile content changes; sync_contact_profiles invokes the persister only when any_changed is true. Unchanged present profiles and confirmed-absent negative-cache refreshes never reach SwiftData, so after restart the stale checkedAtMs is restored and the client can refetch immediately, defeating both the refresh window and the negative-cache backoff across cold starts. Either return a separate needs_persist signal for timestamp-only refreshes or persist on every successful refresh.

suggestion: Contact-request pagination has no per-sweep budget

packages/rs-sdk/src/platform/dashpay/contact_request_queries.rs (line 41)

fetch_contact_requests_paginated drains every matching contactRequest page until exhaustion, accumulating all documents in memory and exposing no continuation. ContactRequest documents are public, so a hostile sender can spam a target identity and force cold-start, restore, unignore, or the recurring DashPay sync loop to spend unbounded time and proof-verification work in one sweep, starving other wallet sync. Add a per-sweep document or elapsed-time cap with a resumable cursor (or at minimum a defensive threshold that aborts and warns).

🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

- [BLOCKING] In `packages/rs-platform-wallet/src/wallet/identity/network/profile.rs`:752-766: Contact-profile chunk query omits required orderBy for In clause
  fetch_contact_profiles_chunk builds `$ownerId In [...]` with `order_by_clauses: vec![]`. Drive treats `In` as a range operator and requires a matching orderBy — the unique ownerId index does not bypass that validation, so DAPI rejects the query with the missing-order-by-for-range error. The caller logs the chunk failure and continues, so the new DashPay contact-profile sync silently fails for every owner and the UI never gets remote profiles. Add an ascending `$ownerId` order clause.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/established_contact.rs`:156-169: established_contact_get_note leaves out_note undefined on early returns
  The C ABI validates that `out_note` is non-null, then performs fallible storage lookup, option unwrap, and CString construction before writing the output pointer. NotFound, no-note, or interior-NUL paths return without defining `*out_note`, so a foreign caller that did not pre-zero the slot can read or free a stale pointer through `platform_wallet_string_free`. The sibling alias getter already pre-nulls its out-param; this getter should match.
- [SUGGESTION] In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ContactRequestsView.swift`:119-128: Incoming requests load sender-controlled avatars before consent
  sync_contact_profiles fetches profiles for `managed.incoming_contact_requests.keys()` (profile.rs:640-642), and ContactRequestsView passes `cachedProfile(row.contactIdentityId)?.avatarUrl` into IncomingRequestRow / DashPayAvatarView, which loads via SwiftUI AsyncImage. An unsolicited sender can publish a per-victim HTTPS avatar URL, send a contact request, and learn when the victim opens the Requests tab from the outbound image fetch — before any accept/ignore action. Suppress remote avatar URLs for pending incoming senders, or drop avatarUrl from the Rust→Swift projection until the contact is established.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/dashpay_profile.rs`:90-185: DashPay profile getters leave owned out-params undefined on early returns
  managed_identity_get_dashpay_profile, platform_wallet_get_dashpay_profile, and platform_wallet_get_contact_profile validate `out_profile`/`out_has_profile` but only assign them after fallible identifier parsing and handle/storage lookups. DashPayProfileFFI contains owned C string pointers later released by dashpay_profile_ffi_free, so a foreign caller that reuses an uninitialized slot can free garbage or stale pointers when a NotFound path fires. Initialize `*out_profile = DashPayProfileFFI::empty()` and `*out_has_profile = false` immediately after pointer validation in all three functions.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/persistence.rs`:2125-2133: InstantSend transaction records are dropped on restore
  Swift PlatformWalletPersistenceHandler.getCoreTxRecordCallback returns context kind 1 along with transaction bytes for InstantSend rows, but FFIPersister maps `context_kind == 1` to `Ok(None)` before decoding them. The DashPay sent-payment reconciliation path treats InstantSend as a display-ready state, so a payment persisted after InstantSend but before any block context disappears across the boundary on relaunch and remains stuck pending. Return a recoverable context when the transaction bytes are present, or expose a separate persisted state reconciliation can consume without the IS-lock blob.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/identity_persistence.rs`:597-641: Confirmed-absent contact profiles cannot delete persisted rows
  `ContactProfileEntry { profile: None }` represents a confirmed absence, but allocate_contact_profile_rows skips those entries entirely (line 610-612). Swift only ever receives present profiles and restore rehydrates every persisted row as `profile: Some(...)`, so if a contact removes their on-chain DashPay profile the stale avatarUrl (potentially attacker-controlled) survives in SwiftData and reappears after cold start, keeping the pre-consent tracking channel alive after the public profile is gone. Project explicit delete/removal intents across the FFI boundary for absent profiles.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/dashpay.rs`:241-245: autoAcceptProof is copied before validating the proof size
  platform_wallet_send_contact_request_with_signer reads `auto_accept_proof_len` bytes into a Vec via `slice::from_raw_parts` before the DashPay proof-size contract (38–102 bytes) is enforced. A malformed binding or import path can trigger oversized allocation or an out-of-bounds FFI read before the proof is rejected. Reject lengths outside 38..=102 before forming the slice.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/persistence.rs`:1493-1504: changeset-end callback failure is not propagated into round_success
  Per-kind callbacks set `round_success = false` on failure, but the final `on_changeset_end_fn` result is only printed to stderr (line 1495-1497). If the Swift commit fails or rolls back during changeset-end, Rust still sees `round_success == true`, returns success, and merges the changeset into its long-lived pending accumulator — durable SwiftData state and Rust in-memory pending state silently diverge. Treat a non-zero changeset-end result as a failed round before the `!round_success` check (and have Swift propagate save failure as a non-zero return).
- [SUGGESTION] In `packages/rs-platform-wallet/src/wallet/identity/network/profile.rs`:588-731: Profile refresh timestamps are not persisted when content is unchanged
  apply_fetched_profile refreshes `checked_at_ms` in memory on every successful fetch but returns `true` only when profile content changes; sync_contact_profiles invokes the persister only when `any_changed` is true. Unchanged present profiles and confirmed-absent negative-cache refreshes never reach SwiftData, so after restart the stale `checkedAtMs` is restored and the client can refetch immediately, defeating both the refresh window and the negative-cache backoff across cold starts. Either return a separate `needs_persist` signal for timestamp-only refreshes or persist on every successful refresh.
- [SUGGESTION] In `packages/rs-sdk/src/platform/dashpay/contact_request_queries.rs`:41-112: Contact-request pagination has no per-sweep budget
  fetch_contact_requests_paginated drains every matching contactRequest page until exhaustion, accumulating all documents in memory and exposing no continuation. ContactRequest documents are public, so a hostile sender can spam a target identity and force cold-start, restore, unignore, or the recurring DashPay sync loop to spend unbounded time and proof-verification work in one sweep, starving other wallet sync. Add a per-sweep document or elapsed-time cap with a resumable cursor (or at minimum a defensive threshold that aborts and warns).

Inline dry-run could not load the GitHub PR diff because this PR exceeds GitHub's 20,000-line diff limit, so I posted the same verified findings as a top-level review body.

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Incremental review of delta a425f70..c583c78: prior-1 (contact-profile chunk query missing orderBy) is FIXED with an extracted contact_profiles_chunk_query plus regression test. prior-5 is now INTENTIONALLY DEFERRED — the code comment at persistence.rs:2127-2132 explicitly documents that InstantSend rows are treated as miss so the SPV wait path completes the proof. The remaining eight prior findings (prior-2, prior-3, prior-4, prior-6, prior-7, prior-8, prior-9, prior-10) are STILL VALID and carried forward; no new in-scope issues were introduced in this delta. Only suggestion-level FFI/security/correctness hardening remains, so the review action is COMMENT.

🟡 8 suggestion(s)

1 additional finding(s) omitted (not in diff).

🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-platform-wallet-ffi/src/dashpay.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/dashpay.rs:241-245: Validate auto_accept_proof length before crossing the FFI boundary
  `platform_wallet_send_contact_request_with_signer` calls `std::slice::from_raw_parts(auto_accept_proof, auto_accept_proof_len).to_vec()` as soon as the pointer is non-null and the length is non-zero. The DashPay proof-size contract (38–102 bytes) is only enforced later inside the SDK, after a full copy. A malformed binding or buggy/hostile caller passing an oversized or inconsistent (ptr, len) pair triggers an oversized allocation or an out-of-bounds FFI read before validation runs. Enforce the size range at the entry point, before constructing the slice, so the boundary stays safe regardless of downstream checks.

In `packages/rs-platform-wallet-ffi/src/persistence.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/persistence.rs:1493-1504: Fold on_changeset_end_fn failure into round_success
  Per-kind callbacks correctly set `round_success = false` on failure, but `on_changeset_end_fn`'s non-zero return is only printed via `eprintln!`. If the host's COMMIT for the round fails or rolls back at changeset-end, Rust still observes `round_success == true`, returns `Ok(())`, and merges the in-memory changeset into `pending` — leaving Rust's view of persisted state strictly ahead of what the backing store actually committed. Downgrade `round_success` on a non-zero changeset-end result so the existing `!round_success` branch triggers the rollback path.

In `packages/rs-platform-wallet-ffi/src/established_contact.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/established_contact.rs:156-169: Initialize *out_note before fallible work in established_contact_get_note
  `established_contact_get_note` validates `out_note` is non-null, then runs three fallible stages (storage `with_item` → `Option` → `CString::new`) before assigning `*out_note`. Each early return leaves the output pointer untouched. The sibling `established_contact_get_alias` at line 108 already establishes the right contract (`*out_alias = std::ptr::null_mut();` immediately after `check_ptr!`); apply the same one-liner here so a C caller that did not pre-zero its slot cannot observe stale memory and pass it to `platform_wallet_string_free`.

In `packages/rs-platform-wallet-ffi/src/dashpay_profile.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/dashpay_profile.rs:88-185: Initialize DashPay profile out-params before fallible work
  `managed_identity_get_dashpay_profile`, `platform_wallet_get_dashpay_profile`, and `platform_wallet_get_contact_profile` all validate `out_profile`/`out_has_profile` with `check_ptr!`, then perform fallible identifier parsing and storage lookups via `unwrap_result_or_return!` / `unwrap_option_or_return!` before assigning. `DashPayProfileFFI` owns four C-string pointers, so a C caller that frees a partially-populated struct on error frees stale or garbage pointers. Write `*out_profile = DashPayProfileFFI::empty(); *out_has_profile = false;` immediately after the null-checks in all three entry points so every error path is deterministic and safe to free.

In `packages/rs-platform-wallet-ffi/src/identity_persistence.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/identity_persistence.rs:597-641: Confirmed-absent contact profiles cannot delete persisted Swift rows
  `allocate_contact_profile_rows` `continue`s past every `ContactProfileEntry { profile: None }`, with the rationale that the negative cache rebuilds on the next sweep. But the FFI re-emits the full `contact_profile_rows` array on every persist, and Swift's persistence handler upserts only the rows it received — a previously-present-now-absent contact (someone who deleted their profile, or a now-confirmed absence) is invisible to Swift, so a stale display name / avatar URL row survives restarts indefinitely. Either emit absent entries with an `is_present: false` discriminator so Swift can DELETE, or pair the array with a per-row tombstone list.

In `packages/rs-platform-wallet/src/wallet/identity/network/profile.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/profile.rs:588-731: Profile refresh timestamps are lost across restart when content is unchanged
  `apply_fetched_profile` refreshes `checked_at_ms` in memory on every successful fetch but returns `true` only when profile *content* changes (line 594 compares profiles, not timestamps). `sync_contact_profiles` invokes the persister only when `any_changed` is true (line 718). Result: unchanged present profiles and confirmed-absent negative-cache refreshes — i.e. the steady-state case — keep their refreshed timestamp only in RAM. After restart the persisted `checked_at_ms` reverts to the last content-changing sweep, so `should_fetch_profile` over-refetches on cold start and re-pulls every cached profile (including unsolicited senders'). Persist on timestamp-only refresh past a debounce, or stamp `checked_at_ms` from a separate per-owner heartbeat row.

In `packages/rs-sdk/src/platform/dashpay/contact_request_queries.rs`:
- [SUGGESTION] packages/rs-sdk/src/platform/dashpay/contact_request_queries.rs:41-111: Contact-request pagination has no per-sweep budget
  `fetch_contact_requests_paginated` drains every page until exhaustion and returns one combined `ContactRequestDocuments` map with no continuation token. ContactRequest documents are public and freely indexable by `toUserId`, so a hostile sender can spam a target identity with cheap throwaway requests and force every cold-start / restore sweep to fetch and hold the entire spam set in memory before any per-row logic runs. With the new 15s `DashPaySyncManager` cadence this becomes a recurring memory-amplification surface. Add a per-sweep page or document budget and surface a continuation cursor so the next sweep resumes instead of re-paginating from scratch.

In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ContactRequestsView.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ContactRequestsView.swift:119-128: Incoming-request rows fetch sender-controlled avatar URLs before consent
  `sync_contact_profiles` now refreshes profiles for `incoming_contact_requests.keys()` every sweep, and `ContactRequestsView` passes `cachedProfile(row.contactIdentityId)?.avatarUrl` into `IncomingRequestRow` / `DashPayAvatarView`, which loads via SwiftUI `AsyncImage`. An unsolicited contact request therefore triggers an unauthenticated HTTPS GET to a URL the sender chose, before the user has accepted — leaking the recipient's IP and approximate online time and giving the sender a tracking pixel into anyone who opens the Requests tab. The `is_valid_avatar_url` filter only constrains the scheme/extension, not who controls the URL. Suppress the avatar (use the placeholder) or proxy through an allow-list until the request is accepted.

@@ -239,14 +245,26 @@ pub unsafe extern "C" fn platform_wallet_send_contact_request_with_signer(
};

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: Validate auto_accept_proof length before crossing the FFI boundary

platform_wallet_send_contact_request_with_signer calls std::slice::from_raw_parts(auto_accept_proof, auto_accept_proof_len).to_vec() as soon as the pointer is non-null and the length is non-zero. The DashPay proof-size contract (38–102 bytes) is only enforced later inside the SDK, after a full copy. A malformed binding or buggy/hostile caller passing an oversized or inconsistent (ptr, len) pair triggers an oversized allocation or an out-of-bounds FFI read before validation runs. Enforce the size range at the entry point, before constructing the slice, so the boundary stays safe regardless of downstream checks.

Suggested change
};
let proof: Option<Vec<u8>> = if auto_accept_proof.is_null() || auto_accept_proof_len == 0 {
None
} else {
if !(38..=102).contains(&auto_accept_proof_len) {
return PlatformWalletFFIResult::err(
PlatformWalletFFIResultCode::InvalidArgument,
"auto_accept_proof length must be between 38 and 102 bytes",
);
}
Some(std::slice::from_raw_parts(auto_accept_proof, auto_accept_proof_len).to_vec())
};

source: ['claude', 'codex']

@@ -1459,6 +1504,20 @@ impl PlatformWalletPersistence for FFIPersister {
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: Fold on_changeset_end_fn failure into round_success

Per-kind callbacks correctly set round_success = false on failure, but on_changeset_end_fn's non-zero return is only printed via eprintln!. If the host's COMMIT for the round fails or rolls back at changeset-end, Rust still observes round_success == true, returns Ok(()), and merges the in-memory changeset into pending — leaving Rust's view of persisted state strictly ahead of what the backing store actually committed. Downgrade round_success on a non-zero changeset-end result so the existing !round_success branch triggers the rollback path.

Suggested change
}
if let Some(cb) = self.callbacks.on_changeset_end_fn {
let result = unsafe { cb(self.callbacks.context, wallet_id.as_ptr(), round_success) };
if result != 0 {
eprintln!("Changeset-end callback returned error code {}", result);
round_success = false;
}
}

source: ['claude', 'codex']

Comment on lines 144 to +185
PlatformWalletFFIResult::ok()
}

/// Read the cached profile of a **contact** (by contact identity id) under
/// the given owner identity. `out_has_profile` is false when the owner has no
/// cached entry for that contact, or the entry is confirmed-absent (the
/// contact published no profile on Platform). Populated by the background
/// contact-profile sync; covers established contacts and pending senders.
#[no_mangle]
pub unsafe extern "C" fn platform_wallet_get_contact_profile(
wallet_handle: Handle,
owner_identity_id: *const u8,
contact_identity_id: *const u8,
out_profile: *mut DashPayProfileFFI,
out_has_profile: *mut bool,
) -> PlatformWalletFFIResult {
check_ptr!(out_profile);
check_ptr!(out_has_profile);

let owner = unwrap_result_or_return!(unsafe { read_identifier(owner_identity_id) });
let contact = unwrap_result_or_return!(unsafe { read_identifier(contact_identity_id) });

let option = PLATFORM_WALLET_STORAGE.with_item(wallet_handle, |wallet| {
let wm = wallet.wallet_manager().blocking_read();
let info = wm.get_wallet_info(&wallet.wallet_id())?;
info.identity_manager
.managed_identity(&owner)
.and_then(|m| m.contact_profiles.get(&contact).cloned())
});
let entry = unwrap_option_or_return!(option);
match entry.and_then(|e| e.profile) {
Some(profile) => unsafe {
*out_profile = DashPayProfileFFI::from_profile(&profile);
*out_has_profile = true;
},
None => unsafe {
*out_profile = DashPayProfileFFI::empty();
*out_has_profile = false;
},
}
PlatformWalletFFIResult::ok()
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: Initialize DashPay profile out-params before fallible work

managed_identity_get_dashpay_profile, platform_wallet_get_dashpay_profile, and platform_wallet_get_contact_profile all validate out_profile/out_has_profile with check_ptr!, then perform fallible identifier parsing and storage lookups via unwrap_result_or_return! / unwrap_option_or_return! before assigning. DashPayProfileFFI owns four C-string pointers, so a C caller that frees a partially-populated struct on error frees stale or garbage pointers. Write *out_profile = DashPayProfileFFI::empty(); *out_has_profile = false; immediately after the null-checks in all three entry points so every error path is deterministic and safe to free.

source: ['claude', 'codex']

Comment on lines +597 to +641
fn allocate_contact_profile_rows(
contact_profiles: &std::collections::BTreeMap<
dpp::prelude::Identifier,
platform_wallet::ContactProfileEntry,
>,
) -> (*const ContactProfileRowFFI, usize) {
if contact_profiles.is_empty() {
return (ptr::null(), 0);
}
let mut rows: Vec<ContactProfileRowFFI> = Vec::with_capacity(contact_profiles.len());
for (contact_id, entry) in contact_profiles {
// Skip confirmed-absent entries — the negative cache is not
// persisted; it rebuilds on the next sweep.
let Some(profile) = entry.profile.as_ref() else {
continue;
};
let (avatar_hash, avatar_hash_present) = match profile.avatar_hash {
Some(h) => (h, true),
None => ([0u8; 32], false),
};
let (avatar_fingerprint, avatar_fingerprint_present) = match profile.avatar_fingerprint {
Some(f) => (f, true),
None => ([0u8; 8], false),
};
rows.push(ContactProfileRowFFI {
contact_id: contact_id.to_buffer(),
display_name: optional_c_string(profile.display_name.as_deref()),
bio: optional_c_string(profile.bio.as_deref()),
avatar_url: optional_c_string(profile.avatar_url.as_deref()),
avatar_hash,
avatar_hash_present,
avatar_fingerprint,
avatar_fingerprint_present,
public_message: optional_c_string(profile.public_message.as_deref()),
checked_at_ms: entry.checked_at_ms,
});
}
if rows.is_empty() {
// Every entry was confirmed-absent — nothing present to carry.
return (ptr::null(), 0);
}
let count = rows.len();
let rows_ptr = Box::into_raw(rows.into_boxed_slice()) as *const ContactProfileRowFFI;
(rows_ptr, count)
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: Confirmed-absent contact profiles cannot delete persisted Swift rows

allocate_contact_profile_rows continues past every ContactProfileEntry { profile: None }, with the rationale that the negative cache rebuilds on the next sweep. But the FFI re-emits the full contact_profile_rows array on every persist, and Swift's persistence handler upserts only the rows it received — a previously-present-now-absent contact (someone who deleted their profile, or a now-confirmed absence) is invisible to Swift, so a stale display name / avatar URL row survives restarts indefinitely. Either emit absent entries with an is_present: false discriminator so Swift can DELETE, or pair the array with a per-row tombstone list.

source: ['claude', 'codex']

Comment on lines +588 to +731
.all_identities()
.into_iter()
.map(|i| i.id())
.collect();

own.iter()
.filter_map(|owner_id| {
let managed = info.identity_manager.managed_identity(owner_id)?;
let mut targets: std::collections::BTreeSet<Identifier> =
managed.established_contacts.keys().copied().collect();
targets.extend(managed.incoming_contact_requests.keys().copied());
let to_fetch: Vec<Identifier> = targets
.into_iter()
.filter(|id| !own.contains(id))
.filter(|id| should_fetch_profile(managed.contact_profiles.get(id), now_ms))
.collect();
(!to_fetch.is_empty()).then_some((*owner_id, to_fetch))
})
.collect()
};

if plan.is_empty() {
return Ok(0);
}

// 2. Fetch (no guard held). Per chunk: one `In` query over ≤IN_CAP
// owner ids; a chunk failure logs and continues so the others
// still land. An id present in the chunk but absent from the
// result is confirmed-absent (cached as `None` — the negative
// cache).
// One owner's fetched contacts: each contact id paired with its profile, or
// `None` when confirmed-absent (the negative cache).
type OwnerContactProfiles = Vec<(Identifier, Option<DashPayProfile>)>;
let mut results: Vec<(Identifier, OwnerContactProfiles)> = Vec::new();
for (owner_id, to_fetch) in plan {
let mut owner_results: OwnerContactProfiles = Vec::new();
for chunk in to_fetch.chunks(CONTACT_PROFILE_IN_CAP) {
match self
.fetch_contact_profiles_chunk(&dashpay_contract, chunk)
.await
{
Ok(found) => {
for id in chunk {
owner_results.push((*id, found.get(id).cloned().flatten()));
}
}
Err(e) => {
tracing::warn!(
owner = %owner_id,
error = %e,
"Failed to fetch a contact-profile chunk; will retry next sweep"
);
}
}
}
if !owner_results.is_empty() {
results.push((owner_id, owner_results));
}
}

// 3. Under the write guard: full-replace, persist-on-change.
let mut written = 0u32;
{
let mut wm = self.wallet_manager.write().await;
let Some(info) = wm.get_wallet_info_mut(&self.wallet_id) else {
return Ok(0);
};
for (owner_id, owner_results) in results {
let Some(managed) = info.identity_manager.managed_identity_mut(&owner_id) else {
continue;
};
let mut any_changed = false;
for (contact_id, profile) in owner_results {
if apply_fetched_profile(
&mut managed.contact_profiles,
contact_id,
profile,
now_ms,
) {
written += 1;
any_changed = true;
}
}
// Persist one changeset per owner, only when something changed —
// the refetch-all-each-sweep first cut stays a persistence
// fixpoint. A failed store self-heals on the next sweep.
if any_changed {
if let Err(e) = self.persister.store(managed.snapshot_changeset().into()) {
tracing::warn!(
owner = %owner_id,
error = %e,
"Failed to persist contact profiles; will retry next sweep"
);
}
}
}
}

Ok(written)
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: Profile refresh timestamps are lost across restart when content is unchanged

apply_fetched_profile refreshes checked_at_ms in memory on every successful fetch but returns true only when profile content changes (line 594 compares profiles, not timestamps). sync_contact_profiles invokes the persister only when any_changed is true (line 718). Result: unchanged present profiles and confirmed-absent negative-cache refreshes — i.e. the steady-state case — keep their refreshed timestamp only in RAM. After restart the persisted checked_at_ms reverts to the last content-changing sweep, so should_fetch_profile over-refetches on cold start and re-pulls every cached profile (including unsolicited senders'). Persist on timestamp-only refresh past a debounce, or stamp checked_at_ms from a separate per-owner heartbeat row.

source: ['claude', 'codex']

Comment on lines +41 to 111
async fn fetch_contact_requests_paginated(
&self,
filter_field: &str,
identity_id: Identifier,
limit: Option<u32>,
after_created_at: Option<u64>,
) -> Result<ContactRequestDocuments, Error> {
// Fetch the DashPay contract
let dashpay_contract = self.fetch_dashpay_contract().await?;

// Query for sent contact requests (where this identity is the owner)
// Note: We need to filter by $ownerId to get only this identity's sent requests
let query = DocumentQuery {
select: drive::query::SelectProjection::documents(),
data_contract: dashpay_contract,
document_type_name: "contactRequest".to_string(),
where_clauses: vec![WhereClause {
field: "$ownerId".to_string(),
operator: WhereOperator::Equal,
value: platform_value!(identity_id),
}],
group_by: vec![],
having: vec![],
order_by_clauses: vec![],
limit: limit.unwrap_or(100),
start: None,
};
let mut where_clauses = vec![WhereClause {
field: filter_field.to_string(),
operator: WhereOperator::Equal,
value: platform_value!(identity_id),
}];
if let Some(after) = after_created_at {
where_clauses.push(WhereClause {
field: "$createdAt".to_string(),
operator: WhereOperator::GreaterThan,
value: platform_value!(after),
});
}

let mut all: ContactRequestDocuments = Default::default();
let mut start: Option<Start> = None;

// Fetch the documents
Document::fetch_many(self, query).await
loop {
let query = DocumentQuery {
select: drive::query::SelectProjection::documents(),
data_contract: dashpay_contract.clone(),
document_type_name: "contactRequest".to_string(),
where_clauses: where_clauses.clone(),
group_by: vec![],
having: vec![],
// Load-bearing: a bare secondary-index equality with no
// order-by is silently proven ABSENT by drive (observed
// against drive 4.0.0-rc.2: `toUserId ==` returned a verified
// empty result for an existing document). The clause also
// pins the query to the contract's `(field, $createdAt)`
// index, giving the deterministic order pagination relies on.
order_by_clauses: vec![OrderClause {
field: "$createdAt".to_string(),
ascending: true,
}],
limit: CONTACT_REQUEST_PAGE_SIZE,
start: start.clone(),
};

let page = Document::fetch_many(self, query).await?;
let page_len = page.len();
// The last document id in query order seeds the next page's
// cursor (distinct from the `$createdAt` high-water the caller
// tracks — this id cursor is ephemeral, per-loop). Relies on
// `Documents` being insertion-ordered (`IndexMap`) so `keys().last()`
// is the `$createdAt`-ascending last doc; a `BTreeMap` here would
// silently reorder by doc id and break pagination.
let last_id = page.keys().last().copied();
for (id, doc) in page {
all.insert(id, doc);
}

// A short page proves exhaustion (a full page may have more).
if page_len < CONTACT_REQUEST_PAGE_SIZE as usize {
break;
}
match last_id {
Some(id) => start = Some(Start::StartAfter(id.to_buffer().to_vec())),
None => break,
}
}

Ok(all)
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: Contact-request pagination has no per-sweep budget

fetch_contact_requests_paginated drains every page until exhaustion and returns one combined ContactRequestDocuments map with no continuation token. ContactRequest documents are public and freely indexable by toUserId, so a hostile sender can spam a target identity with cheap throwaway requests and force every cold-start / restore sweep to fetch and hold the entire spam set in memory before any per-row logic runs. With the new 15s DashPaySyncManager cadence this becomes a recurring memory-amplification surface. Add a per-sweep page or document budget and surface a continuation cursor so the next sweep resumes instead of re-paginating from scratch.

source: ['claude', 'codex']

Comment on lines +119 to +128
ForEach(incomingPending, id: \.contactIdentityId) { row in
IncomingRequestRow(
displayName: displayName(for: row.contactIdentityId),
avatarUrl: cachedProfile(row.contactIdentityId)?.avatarUrl,
createdAtMillis: row.createdAtMillis,
isInFlight: inFlightIds.contains(row.contactIdentityId),
errorMessage: rowErrors[row.contactIdentityId],
onAccept: { accept(contactId: row.contactIdentityId) },
onIgnore: { ignore(contactId: row.contactIdentityId) }
)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: Incoming-request rows fetch sender-controlled avatar URLs before consent

sync_contact_profiles now refreshes profiles for incoming_contact_requests.keys() every sweep, and ContactRequestsView passes cachedProfile(row.contactIdentityId)?.avatarUrl into IncomingRequestRow / DashPayAvatarView, which loads via SwiftUI AsyncImage. An unsolicited contact request therefore triggers an unauthenticated HTTPS GET to a URL the sender chose, before the user has accepted — leaking the recipient's IP and approximate online time and giving the sender a tracking pixel into anyone who opens the Requests tab. The is_valid_avatar_url filter only constrains the scheme/extension, not who controls the URL. Suppress the avatar (use the placeholder) or proxy through an allow-list until the request is accepted.

source: ['claude', 'codex']

shumkov and others added 5 commits June 23, 2026 22:33
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ds-unlock signal, Rust+FFI)

Foundation for the needs-unlock / verify-failed UI signal (DashPay seedless
wallets). Surfaces how many contacts are waiting for a Keychain unlock to
finish payment-account setup, so the UI can show a banner instead of the
current silent print().

- platform-wallet: `IdentityWallet::pending_contact_crypto_count()` reads the
  in-memory deferred-crypto queue via `count_account_build_ops`, counting only
  RegisterReceiving / RegisterExternal. ContactInfoDecrypt is excluded because
  it is re-enqueued on every signerless sweep (no already-decrypted gate), so
  counting it would make the signal a permanent ">0" and re-trip the banner
  ~15s after every unlock. Account-build ops converge to 0 once drained.
- platform-wallet-ffi: `platform_wallet_pending_contact_crypto_count` getter
  (signerless, pollable), mirroring the drain FFI minus the signer.
- Fix doc-rot on PlatformWalletInfo.pending_contact_crypto: cold-load restore
  is blocked upstream; the queue is in-memory + re-enqueued, not restored.

Tests (the exclusion is non-tautological — fails against a naive len()):
- account_build_count_excludes_contact_info_decrypt (platform-wallet)
- pending_contact_crypto_count null-out / unknown-handle (platform-wallet-ffi)

Design + 4-lens review in docs/dashpay/NEEDS_UNLOCK_SIGNAL_SPEC.md.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Consumes the new platform_wallet_pending_contact_crypto_count getter and the
existing verify/drain paths to show a per-wallet banner instead of the silent
print() that was the only signal before.

PlatformWalletManager:
- DashPayUnlockStatus (one Equatable per-wallet snapshot: pendingAccountBuilds,
  seedMismatch, draining) + @published dashPayUnlockStatus, keyed by walletId.
  One struct, not parallel dicts, per the one-snapshot-per-wallet convention.
- pendingAccountBuildCount(for:) thin FFI wrapper; refreshed in the 1 Hz poller
  (per-wallet, gated on change) with ghost-key pruning for unloaded wallets.
- seedMismatch set from the verify FFI result, scoped to that call only — so the
  32-byte walletId precondition (also .invalidParameter) can't be mistaken for a
  seed mismatch. Set at unlock; loadFromPersistor's handling is unchanged.
- drain: an in-flight guard (don't stack a second drain on a banner Unlock tap),
  a draining flag for the UI, and a MainActor hop so the detached drain's
  failure lands on lastError + clears draining (no more print()-only swallow).
- deleteWallet purges the status so a re-created wallet (deterministic id) can't
  inherit a stale banner.

DashPayTabView: a banner between the balance row and the segment Picker.
Priority red seedMismatch (signing disabled) > orange draining ("Finishing…",
no action) > orange "N contact(s) waiting to finish setup" + Unlock. Count is
wallet-scoped (may include sibling identities), so copy says "waiting."

Design + 4-lens review: docs/dashpay/NEEDS_UNLOCK_SIGNAL_SPEC.md.
SwiftDashSDK + SwiftExampleApp build green (iPhone 17 Pro sim).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… verified)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The API-narrowing prerequisite for removing it is already done (all DashPay
signer params are concrete KeychainSigner; zero any-Signer call sites), leaving
only a harmless ~6-line vestigial canSign shim. Not worth a churn commit.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Incremental + cumulative review at 8b4f110. The latest delta (pending_contact_crypto_count FFI + Swift needs-unlock banner) is small, well-formed, tested, and introduces no new in-scope defects. All eight prior findings against c583c78 remain STILL VALID against head — none of the cited lines were modified by this delta. They are carried forward unchanged. No findings warrant blocking; review_action is COMMENT.

🟡 8 suggestion(s)

GitHub cannot serve this PR diff through the review API because it exceeds the 20,000-line diff limit, so I’m posting the verified findings as a top-level exact-SHA review body instead of inline comments.

Findings

SUGGESTION [carried prior-1, STILL VALID] Validate auto_accept_proof length before copying across the FFI boundary

packages/rs-platform-wallet-ffi/src/dashpay.rs:241-245

Verified at head: platform_wallet_send_contact_request_with_signer still calls std::slice::from_raw_parts(auto_accept_proof, auto_accept_proof_len).to_vec() as soon as the pointer is non-null and the length is non-zero. The DashPay 38–102 byte proof contract is enforced only inside the Rust SDK validation downstream, after the unsafe read and allocation have already happened. A malformed C ABI caller passing auto_accept_proof_len = isize::MAX (or a short buffer with a large length) forces an oversized allocation or out-of-bounds read across the boundary before any check runs. Add a (38..=102).contains(&auto_accept_proof_len) precondition immediately after the null/zero gate and return ErrorInvalidParameter.

let proof: Option<Vec<u8>> = if auto_accept_proof.is_null() || auto_accept_proof_len == 0 {
        None
    } else {
        if !(38..=102).contains(&auto_accept_proof_len) {
            return PlatformWalletFFIResult::err(
                PlatformWalletFFIResultCode::ErrorInvalidParameter,
                format!(
                    "auto_accept_proof_len must be between 38 and 102 bytes, got {}",
                    auto_accept_proof_len
                ),
            );
        }
        Some(std::slice::from_raw_parts(auto_accept_proof, auto_accept_proof_len).to_vec())
    };
SUGGESTION [carried prior-2, STILL VALID] Fold on_changeset_end_fn failure into round_success

packages/rs-platform-wallet-ffi/src/persistence.rs:1493-1504

Verified at head (persistence.rs:1493–1497): per-kind callbacks correctly set round_success = false on failure, but on_changeset_end_fn's non-zero return is only eprintln!'d and is not folded into round_success. The end-of-round callback is documented as the host's COMMIT/ROLLBACK signal. If the host's atomic SwiftData commit fails or rolls back at this exact step, Rust still observes round_success == true, returns Ok(()), merges the changeset into pending (lines 1521–1525), and fires on_store_fn — so an in-memory wallet mutation is treated as durably persisted despite the host-side rollback. Set round_success = false (in addition to logging) when the end-of-round callback returns non-zero, so the existing if !round_success early-return triggers.

if let Some(cb) = self.callbacks.on_changeset_end_fn {
            let result = unsafe { cb(self.callbacks.context, wallet_id.as_ptr(), round_success) };
            if result != 0 {
                eprintln!("Changeset-end callback returned error code {}", result);
                round_success = false;
            }
        }
SUGGESTION [carried prior-3, STILL VALID] Initialize *out_note before fallible work in established_contact_get_note

packages/rs-platform-wallet-ffi/src/established_contact.rs:156-169

Verified at head. After check_ptr!(out_note), established_contact_get_note runs three fallible stages (with_item storage lookup, Option unwrap for the absent-note case, CString::new) before assigning *out_note. Each early return via unwrap_*_or_return! leaves the caller's out-pointer untouched. The sibling established_contact_get_alias (lines 103–117) correctly sets *out_alias = std::ptr::null_mut() immediately after the null check; align get_note with that pattern so C/Swift callers don't observe stale pointers on error paths.

check_ptr!(out_note);
    unsafe { *out_note = std::ptr::null_mut() };

    let option =
        ESTABLISHED_CONTACT_STORAGE.with_item(contact_handle, |contact| contact.note.clone());
    let option = unwrap_option_or_return!(option);
    let note = unwrap_option_or_return!(option);
    let c_str = unwrap_result_or_return!(std::ffi::CString::new(note));
    unsafe { *out_note = c_str.into_raw() };
    PlatformWalletFFIResult::ok()
SUGGESTION [carried prior-4, STILL VALID] Initialize DashPay profile out-params before fallible work

packages/rs-platform-wallet-ffi/src/dashpay_profile.rs:88-185

Verified at head. managed_identity_get_dashpay_profile (88–112), platform_wallet_get_dashpay_profile (114–145), and platform_wallet_get_contact_profile (152–185) all check_ptr! out_profile/out_has_profile and only assign them on the success branch of match. Any early return through unwrap_option_or_return!/unwrap_result_or_return! (handle missing, malformed identifier, managed identity not found) leaves the owned DashPayProfileFFI (which holds four CString::into_raw pointers on the success path) and the bool undefined. C/Swift callers must defensively pre-zero — make the contract self-enforcing by initializing *out_profile = DashPayProfileFFI::empty() and *out_has_profile = false immediately after the null checks, mirroring get_alias. The newly added platform_wallet_pending_contact_crypto_count (dashpay.rs:670–686) follows the same leave-untouched-on-error shape and is explicitly pinned by a unit test, so the FFI surface is now inconsistent across getters; standardize on pre-init while you are here.

SUGGESTION [carried prior-5, STILL VALID] Confirmed-absent contact profiles cannot delete persisted Swift rows

packages/rs-platform-wallet-ffi/src/identity_persistence.rs:597-641

Verified at head. allocate_contact_profile_rows (identity_persistence.rs:607–612) still continues past every ContactProfileEntry { profile: None }. The negative cache rebuilds in Rust on the next sweep — but the FFI re-emits the full contact_profile_rows array on every persist and Swift's persistence handler upserts the rows it receives; confirmed absence is never delivered as a delete or tombstone. A profile row persisted before a contact removed/rotated their profile therefore survives indefinitely in SwiftData, including a stale sender-controlled avatar_url (compounds the privacy concern in prior-8). Either emit an absent marker row (e.g. present == false) and have Swift treat it as a delete, or carry the absent ids in a sidecar tombstone slice.

SUGGESTION [carried prior-6, STILL VALID] Profile refresh timestamps are lost across restart when content is unchanged

packages/rs-platform-wallet/src/wallet/identity/network/profile.rs:588-731

Verified at head. apply_fetched_profile (588–603) refreshes checked_at_ms in memory unconditionally but returns changed only when content differs (line 594). sync_contact_profiles only persists when any_changed is true (lines 715–726). Successful unchanged-profile refreshes and negative-cache refreshes therefore never persist the new timestamp. After process restart the wallet replays them as stale on the next sweep, re-running the same In-chunk fetch indefinitely — amplified by every spammed sender. Either persist when checked_at_ms crosses the staleness threshold, or carry a coarser per-owner 'last successful sweep' watermark through the persister.

SUGGESTION [carried prior-7, STILL VALID] Contact-request pagination has no per-sweep budget

packages/rs-sdk/src/platform/dashpay/contact_request_queries.rs:41-111

Verified at head. fetch_contact_requests_paginated still drains every 100-doc page until exhaustion (while page_len == CONTACT_REQUEST_PAGE_SIZE) and accumulates the full result into a single ContactRequestDocuments map before the caller can apply per-row ignore/dedup logic. contactRequest documents are public and indexable by toUserId, so a sender (or many) can cheaply spam a target identity with unsolicited requests. Every cold-start, restore-from-seed sweep, and now every interval of the recurring DashPaySyncManager pays the full memory/CPU cost. Add a per-sweep page or document cap, return a resumable continuation cursor so the next sweep continues from where the previous stopped, and consider a per-(owner, counterparty) high-water mark on the caller side.

SUGGESTION [carried prior-8, STILL VALID] Incoming-request rows fetch sender-controlled avatar URLs before consent

packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ContactRequestsView.swift:119-128

Verified at head. sync_contact_profiles refreshes profiles for incoming_contact_requests.keys() (rs-platform-wallet/.../profile.rs:642) and IncomingRequestRow (ContactRequestsView.swift:119–128) passes cachedProfile(row.contactIdentityId)?.avatarUrl straight into DashPayAvatarView, which renders via SwiftUI AsyncImage(url:). An unsolicited contact request therefore triggers an unauthenticated HTTPS GET to a sender-chosen URL the moment the Requests tab renders — leaking recipient IP, user-agent, and an 'online and looking' timing signal before the user accepts (or even sees) the request. The Rust-side https/length filter narrows the host shape but does not gate disclosure on consent. Gate avatar/network fetches behind explicit Accept (or a user-revealed 'show preview' tap) for incoming-pending senders.

🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/dashpay.rs:241-245`: [carried prior-1, STILL VALID] Validate auto_accept_proof length before copying across the FFI boundary
  Verified at head: `platform_wallet_send_contact_request_with_signer` still calls `std::slice::from_raw_parts(auto_accept_proof, auto_accept_proof_len).to_vec()` as soon as the pointer is non-null and the length is non-zero. The DashPay 38–102 byte proof contract is enforced only inside the Rust SDK validation downstream, after the unsafe read and allocation have already happened. A malformed C ABI caller passing `auto_accept_proof_len = isize::MAX` (or a short buffer with a large length) forces an oversized allocation or out-of-bounds read across the boundary before any check runs. Add a `(38..=102).contains(&auto_accept_proof_len)` precondition immediately after the null/zero gate and return `ErrorInvalidParameter`.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/persistence.rs:1493-1504`: [carried prior-2, STILL VALID] Fold on_changeset_end_fn failure into round_success
  Verified at head (persistence.rs:1493–1497): per-kind callbacks correctly set `round_success = false` on failure, but `on_changeset_end_fn`'s non-zero return is only `eprintln!`'d and is not folded into `round_success`. The end-of-round callback is documented as the host's COMMIT/ROLLBACK signal. If the host's atomic SwiftData commit fails or rolls back at this exact step, Rust still observes `round_success == true`, returns `Ok(())`, merges the changeset into `pending` (lines 1521–1525), and fires `on_store_fn` — so an in-memory wallet mutation is treated as durably persisted despite the host-side rollback. Set `round_success = false` (in addition to logging) when the end-of-round callback returns non-zero, so the existing `if !round_success` early-return triggers.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/established_contact.rs:156-169`: [carried prior-3, STILL VALID] Initialize *out_note before fallible work in established_contact_get_note
  Verified at head. After `check_ptr!(out_note)`, `established_contact_get_note` runs three fallible stages (`with_item` storage lookup, `Option` unwrap for the absent-note case, `CString::new`) before assigning `*out_note`. Each early return via `unwrap_*_or_return!` leaves the caller's out-pointer untouched. The sibling `established_contact_get_alias` (lines 103–117) correctly sets `*out_alias = std::ptr::null_mut()` immediately after the null check; align `get_note` with that pattern so C/Swift callers don't observe stale pointers on error paths.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/dashpay_profile.rs:88-185`: [carried prior-4, STILL VALID] Initialize DashPay profile out-params before fallible work
  Verified at head. `managed_identity_get_dashpay_profile` (88–112), `platform_wallet_get_dashpay_profile` (114–145), and `platform_wallet_get_contact_profile` (152–185) all `check_ptr!` `out_profile`/`out_has_profile` and only assign them on the success branch of `match`. Any early return through `unwrap_option_or_return!`/`unwrap_result_or_return!` (handle missing, malformed identifier, managed identity not found) leaves the owned `DashPayProfileFFI` (which holds four `CString::into_raw` pointers on the success path) and the `bool` undefined. C/Swift callers must defensively pre-zero — make the contract self-enforcing by initializing `*out_profile = DashPayProfileFFI::empty()` and `*out_has_profile = false` immediately after the null checks, mirroring `get_alias`. The newly added `platform_wallet_pending_contact_crypto_count` (dashpay.rs:670–686) follows the same leave-untouched-on-error shape and is explicitly pinned by a unit test, so the FFI surface is now inconsistent across getters; standardize on pre-init while you are here.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/identity_persistence.rs:597-641`: [carried prior-5, STILL VALID] Confirmed-absent contact profiles cannot delete persisted Swift rows
  Verified at head. `allocate_contact_profile_rows` (identity_persistence.rs:607–612) still `continue`s past every `ContactProfileEntry { profile: None }`. The negative cache rebuilds in Rust on the next sweep — but the FFI re-emits the full `contact_profile_rows` array on every persist and Swift's persistence handler upserts the rows it receives; confirmed absence is never delivered as a delete or tombstone. A profile row persisted before a contact removed/rotated their profile therefore survives indefinitely in SwiftData, including a stale sender-controlled `avatar_url` (compounds the privacy concern in prior-8). Either emit an absent marker row (e.g. `present == false`) and have Swift treat it as a delete, or carry the absent ids in a sidecar tombstone slice.
- [SUGGESTION] In `packages/rs-platform-wallet/src/wallet/identity/network/profile.rs:588-731`: [carried prior-6, STILL VALID] Profile refresh timestamps are lost across restart when content is unchanged
  Verified at head. `apply_fetched_profile` (588–603) refreshes `checked_at_ms` in memory unconditionally but returns `changed` only when content differs (line 594). `sync_contact_profiles` only persists when `any_changed` is true (lines 715–726). Successful unchanged-profile refreshes and negative-cache refreshes therefore never persist the new timestamp. After process restart the wallet replays them as stale on the next sweep, re-running the same `In`-chunk fetch indefinitely — amplified by every spammed sender. Either persist when `checked_at_ms` crosses the staleness threshold, or carry a coarser per-owner 'last successful sweep' watermark through the persister.
- [SUGGESTION] In `packages/rs-sdk/src/platform/dashpay/contact_request_queries.rs:41-111`: [carried prior-7, STILL VALID] Contact-request pagination has no per-sweep budget
  Verified at head. `fetch_contact_requests_paginated` still drains every 100-doc page until exhaustion (`while page_len == CONTACT_REQUEST_PAGE_SIZE`) and accumulates the full result into a single `ContactRequestDocuments` map before the caller can apply per-row ignore/dedup logic. `contactRequest` documents are public and indexable by `toUserId`, so a sender (or many) can cheaply spam a target identity with unsolicited requests. Every cold-start, restore-from-seed sweep, and now every interval of the recurring `DashPaySyncManager` pays the full memory/CPU cost. Add a per-sweep page or document cap, return a resumable continuation cursor so the next sweep continues from where the previous stopped, and consider a per-(owner, counterparty) high-water mark on the caller side.
- [SUGGESTION] In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ContactRequestsView.swift:119-128`: [carried prior-8, STILL VALID] Incoming-request rows fetch sender-controlled avatar URLs before consent
  Verified at head. `sync_contact_profiles` refreshes profiles for `incoming_contact_requests.keys()` (rs-platform-wallet/.../profile.rs:642) and `IncomingRequestRow` (ContactRequestsView.swift:119–128) passes `cachedProfile(row.contactIdentityId)?.avatarUrl` straight into `DashPayAvatarView`, which renders via SwiftUI `AsyncImage(url:)`. An unsolicited contact request therefore triggers an unauthenticated HTTPS GET to a sender-chosen URL the moment the Requests tab renders — leaking recipient IP, user-agent, and an 'online and looking' timing signal before the user accepts (or even sees) the request. The Rust-side https/length filter narrows the host shape but does not gate disclosure on consent. Gate avatar/network fetches behind explicit Accept (or a user-revealed 'show preview' tap) for incoming-pending senders.

…oritized

Full DIP-15 + reference-client audit (dashj / android-dashpay / dash-wallet) of
the dormant autoAcceptProof / auto_accept.rs. Verdict: unimplemented in every
reference client (zero refs in dashj; field+unused-builder with 0 callers in
android-dashpay; dormant DB pass-through in dash-wallet whose QR scanner only
does payments). No interoperable counterparty, and DIP-15 leaves verification /
signed-byte serialization / expiry / replay undefined. Our crypto also models
the wrong actor for the scanner side.

Recommendation: don't wire it; keep auto_accept.rs tested+dormant. The shipped,
interoperable 'scan/link to onboard a friend' feature is Invitations (DIP-13
sub-feature 3', dashpay://invite + AssetLock), a separate larger feature.

Findings: docs/dashpay/QR_AUTO_ACCEPT_RESEARCH.md.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Latest delta (8b4f110..c8e4065) is documentation-only (docs/dashpay/QR_AUTO_ACCEPT_RESEARCH.md and TODO.md edits). No executable surface changed, and no new in-scope findings were introduced. All eight prior findings were re-verified against HEAD and are STILL VALID — the code at dashpay.rs:241-245, persistence.rs:1493-1498, established_contact.rs:156-169, dashpay_profile.rs:88-185, identity_persistence.rs:597-641, profile.rs:588-731, contact_request_queries.rs:41-111, and ContactRequestsView.swift:119-128 is byte-identical to 8b4f110. None are blocking; the cumulative recommendation remains COMMENT.

🟡 1 suggestion(s)

1 additional finding(s) omitted (not in diff).

7 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.

🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-platform-wallet-ffi/src/established_contact.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/established_contact.rs:156-169: Initialize *out_note before fallible work in established_contact_get_note
  [Carried prior-3, STILL VALID at c8e40657 — unchanged.] After `check_ptr!(out_note)`, `established_contact_get_note` runs three fallible stages (`with_item` storage lookup, `Option` unwrap for absent-note, `CString::new`) before assigning `*out_note`. Each early return via `unwrap_*_or_return!` leaves the caller's out-pointer untouched. The sibling `established_contact_get_alias` at lines 103–117 correctly sets `*out_alias = std::ptr::null_mut()` immediately after the null check; align `get_note` with that pattern so C/Swift callers do not observe stale pointers (potentially freed via `platform_wallet_string_free`) on error paths.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants